-
Notifications
You must be signed in to change notification settings - Fork 193
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
feat: add normalize_by_update_norm
#958
feat: add normalize_by_update_norm
#958
Conversation
CI is green again 😄 |
Gentle ping: @fabianp |
thanks for the ping, will look into this ASAP |
Hi @SauravMaheshkar , a couple more comments after discussing this issue with the dev team:
what do you think about these comments? do they sound reasonable? |
Hey @fabianp glad to know there's interest. That sounds reasonable. Let me do some refactoring. Since it's going into main, I'll put some more effort in the docstrings as well. Will push soon 😄 |
amazing, thanks! |
@fabianp I've refactored my branch to add the transform to main rather than contrib. Request for Review 😄 |
scale_by_gradient_norm
normalize_by_update_norm
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.
we're getting very close!
One last thing, could you add a doctest (i.e., an example in the docstring) of the new function ? see methods in alias.py
for inspiration :-)
Co-authored-by: Fabian Pedregosa <pedregosa@google.com>
I've added a doctest 👍🏽 @fabianp |
looks good to me, but I forgot to ask you one (hopefully) last thing: please add this new function to the API documentation: https://github.com/google-deepmind/optax/blob/main/docs/api/transformations.rst thanks! 🙏🏼 |
Added in 5c01f6f |
thanks @SauravMaheshkar ! |
duplicated #1000 since I'm having some issues getting this merged due to internal errors |
Typically copybara I assume 😅 |
yup. Finally merged 🎉 |
Thanks a ton. Feels good to have it merged. Now on to #652 |
Reference: #594
This PR aims to add a new
GradientTransformation
which allows to scale by the gradient norm.Request for Review: @fabianp