Add datamodule parameter to lr_find()

[GimmickNG]

What does this PR do?

Fixes #3424 - Adds datamodule support to Trainer.lr_find() - a LightningDataModule can be passed to lr_find().

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
• Did you make sure to update the documentation with your changes?
• Did you write any new necessary tests?
• Did you verify new and existing tests pass locally with your changes?
• If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

[codecov]

Codecov Report

Merging #3425 into master will increase coverage by 8%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #3425    +/-   ##
=======================================
+ Coverage      83%     90%    +8%     
=======================================
  Files         108     108            
  Lines        8807    8386   -421     
=======================================
+ Hits         7284    7584   +300     
+ Misses       1523     802   -721     

[williamFalcon]

does this work?
can you add a test?

[mergify]

This pull request is now in conflict... :(

[GimmickNG]

Yeah, it does - AFAIK all it does is forward the datamodule parameter to the fit() method for the temporary trainer created in lr_find().

I'm not quite sure how to write tests but I'll write them once I figure out how to do so.

[SkafteNicki]

@GimmickNG changes look fine.
For testing, you can write a altered version of this test:
https://github.com/PyTorchLightning/pytorch-lightning/blob/5abf7d91232322e7f87fe65ad490245d9c1894c4/tests/trainer/test_lr_finder.py#L133-L152
where you remove all dataloaders in the model and then feed in a datamodule to the lr_find method. If the search finish the data is passed correctly.

[GimmickNG]

@GimmickNG changes look fine.
For testing, you can write a altered version of this test:

https://github.com/PyTorchLightning/pytorch-lightning/blob/5abf7d91232322e7f87fe65ad490245d9c1894c4/tests/trainer/test_lr_finder.py#L133-L152

where you remove all dataloaders in the model and then feed in a datamodule to the lr_find method. If the search finish the data is passed correctly.

I've added a test for testing the datamodule parameter. I'm not sure if I've done it correctly, though.

[I'd written a test which checked whether passing the datamodule parameter works when calling trainer.tune(model) with Trainer(auto_lr_find=True), because I was under the impression that calling tune() would be different to calling tuner.lr_find() - I've rolled the changes back and simplified the test to the one suggested by @SkafteNicki]

[SkafteNicki]

We are currently doing a lot of internal refactors and the file pytorch_lightning/trainer/lr_finder.py therefore do not exist anymore (the conflicted file). Please redo the changes to this file: pytorch_lightning/tuner/lr_finder.py.

[GimmickNG]

OK.

[edenlightning]

Hey @GimmickNG! Refactors are mostly done, mind taking another look?

[GimmickNG]

Hey @GimmickNG! Refactors are mostly done, mind taking another look?

Sorry, I've been rather busy the past week. Should I create another pull request with the updated version of the fork?

[Borda]

Hey @GimmickNG! Refactors are mostly done, mind taking another look?

Sorry, I've been rather busy the past week. Should I create another pull request with the updated version of the fork?

I think that resolving conflict is fine (rebase on actual master)

[GimmickNG]

Sorry, I can't seem to do that on the web client and I'm rather inexperienced with this. How would I get Git to recognize the file's been moved to a different module during the rebase?

[SkafteNicki]

@GimmickNG I merged master into your code and fixed conflicts. Failing checks seems to be unrelated to PR. Please take a look and make sure the PR is still doing what you intended, then we can go ahead and approve changes.

[GimmickNG]

@GimmickNG I merged master into your code and fixed conflicts. Failing checks seems to be unrelated to PR. Please take a look and make sure the PR is still doing what you intended, then we can go ahead and approve changes.

Thanks! I checked it and trainer.tuner.lr_find(model, datamodule=data_module) works, at least on my end. I think it should be fine to close.

[Borda]

LGTM

[Borda]

@GimmickNG I merged master into your code and fixed conflicts. Failing checks seems to be unrelated to PR. Please take a look and make sure the PR is still doing what you intended, then we can go ahead and approve changes.

Thanks! I checked it and trainer.tuner.lr_find(model, datamodule=data_module) works, at least on my end. I think it should be fine to close.

I would keep it for merge as it adds test for the before/after...

[SkafteNicki]

LGTM

[pep8speaks]

Hello @GimmickNG! Thanks for updating this PR.

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

Comment last updated at 2020-09-30 13:57:10 UTC

[williamFalcon]

Please rebae master...
cc @Borda