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

Add NovoGrad optimizer #385

Closed
wants to merge 7 commits into from
Closed

Add NovoGrad optimizer #385

wants to merge 7 commits into from

Conversation

DT6A
Copy link
Contributor

@DT6A DT6A commented Jul 31, 2022

NovoGrad optimizer was presented in this paper: https://arxiv.org/pdf/1905.11286.pdf
It was used to train the ASR model named Jasper: https://arxiv.org/pdf/1904.03288.pdf

@google-cla
Copy link

google-cla bot commented Jul 31, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@mtthss
Copy link
Collaborator

mtthss commented Aug 23, 2022

There are a couple of conflicts, could you sync and merge so we get this submitted?

@DT6A
Copy link
Contributor Author

DT6A commented Aug 23, 2022

There are a couple of conflicts, could you sync and merge so we get this submitted?

Hello! Yes, I will try to fix as soon as possible

@@ -149,6 +149,7 @@ Gradient Transforms
scale_by_adam
scale_by_belief
scale_by_factored_rms
scale_by_novograd
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you also add novograd to the Common Optimizers section?

mu_dtype: optional `dtype` to be used for the first order accumulator; if
`None` then the `dtype is inferred from `params` and `updates`.
Returns:
An (init_fn, update_fn) tuple.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"A GradientTransformation object."

@hbq1
Copy link
Collaborator

hbq1 commented Oct 19, 2022

Thanks a lot for the contribution!

@hbq1
Copy link
Collaborator

hbq1 commented Oct 19, 2022

@DT6A could you please add a test for the new optimizer?

@DT6A
Copy link
Contributor Author

DT6A commented Oct 19, 2022

@hbq1 is there any reference how the test should be done?

@mkunesch
Copy link
Member

mkunesch commented Oct 19, 2022

Hi! We are in the process of writing instructions on this at the moment - in your case it should be enough to just add the new optimizer to the list of optimizers under test in alias_test.py here. Let me know if you have any questions on this!

@DT6A
Copy link
Contributor Author

DT6A commented Oct 19, 2022

@mkunesch Thanks, added it

@mkunesch
Copy link
Member

mkunesch commented Oct 19, 2022

Thank you!
(If there are details of the optimizer that would be important to test in isolation it would be good to add a unit test for them, but from a quick look at the documentation and code nothing stood out to me)

copybara-service bot pushed a commit that referenced this pull request Oct 19, 2022
PiperOrigin-RevId: 482336516
@hbq1
Copy link
Collaborator

hbq1 commented Oct 20, 2022

Merged! Thanks again for this great contribution 🎉

@hbq1 hbq1 closed this Oct 20, 2022
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.

None yet

4 participants