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.

Sign up for GitHub

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

Jump to bottom

Add Novograd Optimizer #836

Merged
merged 62 commits into from
Jan 12, 2020
Merged

Add Novograd Optimizer #836

merged 62 commits into from
Jan 12, 2020

Conversation

0xshreyash
Copy link
Contributor

@0xshreyash 0xshreyash commented Jan 5, 2020 •
edited
Loading

This PR adds in the Novograd Optimizer for tensorflow/addons.

I have tried to keep my implementation of things as similar to the original implementation as possible.

@0xshreyash 0xshreyash changed the title Novograd Optimizer WIP: Novograd Optimizer Jan 5, 2020
WindQAQ
WindQAQ requested changes Jan 8, 2020
View reviewed changes
Copy link
Member

@WindQAQ WindQAQ left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

tensorflow_addons/optimizers/novograd.py
from __future__ import print_function

import tensorflow as tf
from tensorflow.python.training import training_ops
Copy link
Member

Choose a reason for hiding this comment

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

Seems that private API is the only way to import this. @seanpmorgan Sean, what do you think about this?

Copy link
Member

Choose a reason for hiding this comment

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

Hi @shreyashpatodia, could we add some comments here? Like

# TODO: Find public API alternatives to these
from tensorflow.python.training import training_ops

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

tensorflow_addons/optimizers/novograd.py Outdated Show resolved Hide resolved
tensorflow_addons/optimizers/novograd.py Outdated Show resolved Hide resolved
tensorflow_addons/optimizers/novograd.py Outdated Show resolved Hide resolved
tensorflow_addons/optimizers/novograd.py Show resolved Hide resolved
tensorflow_addons/optimizers/novograd.py Show resolved Hide resolved
tensorflow_addons/optimizers/novograd.py Show resolved Hide resolved
tensorflow_addons/optimizers/novograd_test.py Outdated Show resolved Hide resolved
tensorflow_addons/optimizers/novograd_test.py Outdated Show resolved Hide resolved
tensorflow_addons/optimizers/novograd_test.py Outdated Show resolved Hide resolved
@0xshreyash
Copy link
Contributor Author

@WindQAQ making the tf.equal params come on the same line seemed to have broken sanity-checks. I reverted back the change, for now.

@WindQAQ
Copy link
Member

WindQAQ commented Jan 9, 2020

@WindQAQ making the tf.equal params come on the same line seemed to have broken sanity-checks. I reverted back the change, for now.

I'm fine with this :P Let's trigger the CI again.

Hello @valendin, could you take a look at this to see if you have further problems? Thanks!

@WindQAQ WindQAQ self-requested a review January 12, 2020 00:37
WindQAQ
WindQAQ approved these changes Jan 12, 2020
View reviewed changes
Copy link
Member

@WindQAQ WindQAQ left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@WindQAQ WindQAQ merged commit 10ccec3 into tensorflow:master Jan 12, 2020
@0xshreyash 0xshreyash deleted the novograd_optimizer branch January 12, 2020 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WindQAQ WindQAQ WindQAQ approved these changes

@facaiy facaiy Awaiting requested review from facaiy

@Squadrick Squadrick Awaiting requested review from Squadrick

Assignees
No one assigned
Labels
cla: yes optimizers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@0xshreyash @WindQAQ @googlebot @seanpmorgan @kokoro-team