Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix: Lr finder and hparams compatibility #2821

Merged
merged 10 commits into from
Aug 6, 2020

Conversation

SkafteNicki
Copy link
Member

What does this PR do?

Fixes #1983
Solves compatibility problem between the (new) hparams interface and the learning rate finder

@mergify mergify bot requested a review from a team August 4, 2020 10:25
@codecov
Copy link

codecov bot commented Aug 4, 2020

Codecov Report

Merging #2821 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #2821   +/-   ##
======================================
  Coverage      89%     89%           
======================================
  Files          79      79           
  Lines        7302    7302           
======================================
  Hits         6514    6514           
  Misses        788     788           

@Borda Borda added the bug Something isn't working label Aug 4, 2020
@mergify mergify bot requested a review from a team August 4, 2020 10:54
@mergify mergify bot requested a review from a team August 4, 2020 15:01
@mergify mergify bot requested a review from a team August 5, 2020 00:18
@awaelchli
Copy link
Contributor

awaelchli commented Aug 5, 2020

If setting the attribute failed before, it means that we have no tests for the LRfinder that involves setting the learning rate. I expected such a test case with this fix. What do you think?

@SkafteNicki
Copy link
Member Author

@awaelchli I am pretty sure that there should be test for this, but I will definitely check up on it and add if missing (fix if broken)

@pep8speaks
Copy link

pep8speaks commented Aug 5, 2020

Hello @SkafteNicki! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-08-06 11:31:14 UTC

@SkafteNicki
Copy link
Member Author

@awaelchli you were right, the test did not check this since the base testing model model=EvalModelTemplate() both have an model.learning_rate attribute and a model.hparams.learning_rate attribute with the latter never being checked. I have changed this now.

@nateraw nateraw self-requested a review August 5, 2020 21:52
@nateraw nateraw added the ready PRs ready to be merged label Aug 5, 2020
CHANGELOG.md Outdated Show resolved Hide resolved
tests/utilities/parsing.py Outdated Show resolved Hide resolved
Copy link
Contributor

@awaelchli awaelchli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

solid fix! Thanks @SkafteNicki
I imagine a similar problem exists in the batch size finder, so your helper functions added here could be useful there.

@williamFalcon
Copy link
Contributor

looks like GPU tests are failing?
@awaelchli, @SkafteNicki

@awaelchli
Copy link
Contributor

awaelchli commented Aug 6, 2020

@williamFalcon It looks like these failing tests come from master

@williamFalcon williamFalcon force-pushed the 'fix_hparams_lr_finder' branch from f57e02d to d6a7e43 Compare August 6, 2020 02:00
@Borda
Copy link
Member

Borda commented Aug 6, 2020

if has been rebased (master is fixed) but it is still failing, @SkafteNicki mind check it?

@nateraw any idea why this fails?

FAILED tests/core/test_datamodules.py::test_full_loop_ddp_spawn

@mergify
Copy link
Contributor

mergify bot commented Aug 6, 2020

Great job! =)

@Borda Borda added this to the 0.9.0 milestone Aug 6, 2020
@Borda Borda merged commit 9a40246 into Lightning-AI:master Aug 6, 2020
@SkafteNicki SkafteNicki deleted the 'fix_hparams_lr_finder' branch October 8, 2020 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

auto_lr_find does not work
8 participants