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

implementing changes for exposing Lightning Tuner's LR finder #1609

Merged
merged 15 commits into from
Mar 30, 2023

Conversation

solalatus
Copy link
Contributor

@solalatus solalatus commented Mar 2, 2023

Fixes #1503.

Summary

As proposed in the linked issue here, this PR implements a lightweight setting for the fit() methods of the TorchForecastingModels to just do a "setup" run, and return gracefully without being interrupted and without running the main training loop. This enabled the usage of Lightning Tuner to find optimal learning rates. For this a new method is exposed, that wraps Lightning's method and provides sane defaults.

Other Information

I have not done unittests for this functionality, since I am basically exposing some already present functionality. None the less I included a Notebook in the examples here that demonstrates the usage of the feature.

Updates (from @dennisbader):

  • decouple fit_from_dataset and _train setup from fitting the model. like this we can setup the model without having to fit the model, or having to add a parameter setup_only
  • adds unit tests

@solalatus solalatus requested a review from dennisbader as a code owner March 2, 2023 19:00
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Collaborator

@dennisbader dennisbader left a comment

Choose a reason for hiding this comment

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

Hi @solalatus and thanks for this PR! Looks good already :)

I made a suggestion for adding a new private method that handles setting up the model for training, to avoid adapting the fit API and users having to call fit() with setup_only=True before lr_find.

darts/models/forecasting/torch_forecasting_model.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Mar 20, 2023

Codecov Report

Attention: Patch coverage is 97.05882% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.07%. Comparing base (de94ef4) to head (e7ff956).
Report is 289 commits behind head on master.

Files with missing lines Patch % Lines
...arts/models/forecasting/torch_forecasting_model.py 97.05% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1609      +/-   ##
==========================================
- Coverage   94.14%   94.07%   -0.08%     
==========================================
  Files         125      125              
  Lines       11364    11370       +6     
==========================================
- Hits        10699    10696       -3     
- Misses        665      674       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@madtoinou madtoinou mentioned this pull request Mar 23, 2023
@dennisbader
Copy link
Collaborator

Hi @solalatus, just wanted to check in how it's going with this PR.
We plan to release a new version in the next 2 weeks, and would like to include this one :) (no rush though!)

@solalatus
Copy link
Contributor Author

@dennisbader thanks for checking. Sadly I am completely overwhelmed, so will not be able to make progress on this. Can anyone please take over and push it through the "finish line"?

@dennisbader
Copy link
Collaborator

Yes, I can take over from here. Thanks for the work so far!

Copy link
Collaborator

@madtoinou madtoinou left a comment

Choose a reason for hiding this comment

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

LGTM, I find this approach quite elegant!

dennisbader and others added 2 commits March 29, 2023 17:23
Copy link
Collaborator

@dennisbader dennisbader left a comment

Choose a reason for hiding this comment

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

Looks good now :) Thanks @solalatus for the contribution!

@solalatus
Copy link
Contributor Author

@dennisbader thanks for pushing it through! Cheers!

@dennisbader dennisbader merged commit 5479108 into unit8co:master Mar 30, 2023
alexcolpitts96 pushed a commit to alexcolpitts96/darts that referenced this pull request May 31, 2023
…o#1609)

* implementing changes for exposing Lightning Tuner's LR finder

* decouple dataset/loader generation from training

* improve decoupling

* add unit test and docs

* add plotting from unittest

* remove lr_finder example

* fix minor mistakes in docs

* final updates for PR

* Update darts/tests/models/forecasting/test_torch_forecasting_model.py

Co-authored-by: madtoinou <32447896+madtoinou@users.noreply.github.com>

* fix Tuner import for PTL<2.0.0

---------

Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>
Co-authored-by: madtoinou <32447896+madtoinou@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lightning Tuner functions
4 participants