-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Stepwise LR scheduler #20211
base: master
Are you sure you want to change the base?
Stepwise LR scheduler #20211
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…ytorch-lightning into ddp-strategy-alias
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #20211 +/- ##
=========================================
- Coverage 89% 81% -8%
=========================================
Files 267 264 -3
Lines 23084 23029 -55
=========================================
- Hits 20585 18618 -1967
- Misses 2499 4411 +1912 |
Hii @Borda. Do I need to make any kind of changes in the PR ? |
This looks good, thank you for the contribution @01AbhiSingh Ideally we could add a test to verify the behavior described in #17544. The current test suite can't detect the current change and this is usually a sign of insufficient coverage. Would you be willing to contribute such test? |
Yes, sure let me look into it. |
Hi @lantiga , Do you want a new test written from scratch or need me to make the necessary changes in a preexisting file? All the tests have been passed. If the changes need to be made in a preexisting file, it would be very helpful if you could point out the test in which I need to make the changes, as all the tests have been passed, and due to that, I can't find the test. |
hey @01AbhiSingh sorry for the wait You can take inspiration from:
and add a new test where scheduling goes across epoch boundaries. Maybe @falckt can help too? |
@01AbhiSingh the test looks good! feel free to add it to |
for more information, see https://pre-commit.ci
Done. sorry I was bogged down by few things. But, a lot of new PRs for all those issues coming your way now. |
there's an issue with |
do you mean I need to add fixture to the test function's parameters.
|
That's right, you can also decorate the test with from unittest.mock import patch
...
@patch("torch.optim.lr_scheduler.StepLR.step")
def test_lr_scheduler_step_across_epoch_boundaries(mocked_sched, tmp_path):
... this should work |
Done please check |
Hey @01AbhiSingh can you import Change: from lightning.pytorch import Trainer to from lightning.pytorch import Trainer, LightningModule this should fix the failing test |
…pytorch-lightning into stepwiseLRscheduler
for more information, see https://pre-commit.ci
Yeah, my bad. Forgot to add it even after seeing it. Done, please check. |
This is the test that is currently failing.
should I add this and try to run the test again ? |
Go for it : ) You can also run this kind of test locally with |
I actually tried to run the test locally with the method you suggested but this error keeps showing up Edit: I've solved this problem, will now update the PR only when it's running perfectly on my local environment. Thanks :) Another Edit 😝 : updated the PR please check |
…utils.data import DataLoader, TensorDataset
…pytorch-lightning into stepwiseLRscheduler
for more information, see https://pre-commit.ci
…pytorch-lightning into stepwiseLRscheduler
for more information, see https://pre-commit.ci
Test passing on my local environment but not in the PR in the repo. |
What does this PR do?
Fixes #<17544>
Hii @awaelchli. Can you please verify the changes I made. If they are correct then i will take up and correct any failing tests also.
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:
Reviewer checklist
- [ ] Is this pull request ready for review? (if not, please submit in draft mode) - [ ] Check that all items from **Before submitting** are resolved - [ ] Make sure the title is self-explanatory and the description concisely explains the PR - [ ] Add labels and milestones (and optionally projects) to the PR so it can be classified📚 Documentation preview 📚: https://pytorch-lightning--20211.org.readthedocs.build/en/20211/