-
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
[Refactor] Move reset_on_restart within the loop reset #9561
Conversation
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
c3f3f7e
to
e9a46d0
Compare
Codecov Report
@@ Coverage Diff @@
## master #9561 +/- ##
======================================
Coverage 93% 93%
======================================
Files 180 181 +1
Lines 15092 15170 +78
======================================
+ Hits 14020 14094 +74
- Misses 1072 1076 +4 |
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
…ightning/pytorch-lightning into move_restart_with_loops
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the linked issue is empty and the PR is missing a description. we are missing a motivation why this needs to be changed. Also, what will 2/2 include?
If it's not ready for review, let's mark the PR as draft
Hey @awaelchli, I apologise. I started and was caught into something else. I have added a description and the PR is ready. Best, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand the changes you made to the test I wrote. Can you explain it?
…ightning/pytorch-lightning into move_restart_with_loops
Co-authored-by: Carlos Mocholi <carlossmocholi@gmail.com>
What does this PR do?
Currently, the loop reset_on_restart the progress while reloading the states. This means setting ready, started value to the processed / completed one.
This hidden behaviour can be mis-leading and should be exposed within the reset function of the loop.
Fixes #9562
Does your PR introduce any breaking changes? If yes, please list them.
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃