-
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
[Bugfix] Fix location of unscale
in mixed precision plugin
#9606
Conversation
@justusschock I think this is ready for review now |
unscale
in mixed precision plugin
Codecov Report
@@ Coverage Diff @@
## release/1.4.x #9606 +/- ##
==============================================
- Coverage 93% 88% -4%
==============================================
Files 218 218
Lines 14513 14519 +6
==============================================
- Hits 13425 12814 -611
- Misses 1088 1705 +617 |
5a8d078
to
35d03b8
Compare
Co-authored-by: Sean Naren <[email protected]>
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.
LGTM !
Co-authored-by: thomas chaton <[email protected]>
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.
Review all
What does this PR do?
Fixes #9599, #9330, #9205, #9694
Unscale was called after gradnorm tracking and gradient clipping. Moving it to a different hook in the plugins should fix that.
Alternative solution would be to move gradient tracking and clipping. Finding a solution that works for all plugin combinations might be difficult though.
Probably same purpose as #9287
Does your PR introduce any breaking changes? If yes, please list them.
None
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 🙃
Please don't merge before review from @carmocca