Skip to content

Rename LazyAdamOptimizer to LazyAdam #109

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
Merged
seanpmorgan merged 3 commits into from
Mar 29, 2019
Merged

Rename LazyAdamOptimizer to LazyAdam #109

seanpmorgan merged 3 commits into from
Mar 29, 2019
+21 −22

Conversation

@Squadrick
Copy link
Member

@Squadrick Squadrick commented Mar 28, 2019

Fixes #104

@Squadrick
Rename LazyAdamOptimizer to LazyAdam
e6c8bd2 
* Rename `lazy_adam_optimizer.py` -> `lazy_adam.py`

* Rename `lazy_adam_optimizer_test.py` -> `lazy_adam_test.py`

* Rename LazyAdamOptimizerTest -> LazyAdamTest

* Update README.md

* Update BUILD files
@Squadrick
Refactor LazyAdamOptimizer to LazyAdam in example code
fddeb92
armando-fandango
armando-fandango reviewed Mar 29, 2019
View reviewed changes
Copy link
Contributor

@armando-fandango armando-fandango left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@facaiy facaiy requested a review from seanpmorgan March 29, 2019 04:06
@facaiy facaiy self-assigned this Mar 29, 2019
@facaiy
Copy link
Member

facaiy commented Mar 29, 2019

Thanks, Dheeraj, Armando!
Let's merge it after Sean approves the change cc @seanpmorgan

armando-fandango
armando-fandango previously requested changes Mar 29, 2019
View reviewed changes
Copy link
Contributor

@armando-fandango armando-fandango left a comment

Choose a reason for hiding this comment

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

Seems like its failing the format test: "Use make code-format command to format codes automatically
Format checks FAILED"

@Squadrick
Copy link
Member Author

Squadrick commented Mar 29, 2019

@armando-fandango I formatted the python file, it should pass the tests now.

@Squadrick
Copy link
Member Author

Squadrick commented Mar 29, 2019

@facaiy

On lazy_adam_test.py line 217. If I run yapf it formats the line in a way that fails in pylint.

@facaiy
Copy link
Member

facaiy commented Mar 29, 2019

Dheeraj, do you take make code-format and make sanity-check a try ?https://github.com/tensorflow/addons/blob/master/CONTRIBUTING.md#development-environment

By the way, we always squash all commits into one commit before merging, so it seems not necessary to rebase our commits :-) . (rebase is kind of dangerous and not friendly for reviewer).

@facaiy
Copy link
Member

facaiy commented Mar 29, 2019

What pylint error? You can disable it temporarily https://github.com/tensorflow/addons/blob/master/STYLE_GUIDE.md

@seanpmorgan seanpmorgan merged commit 63bb4b4 into tensorflow:master Mar 29, 2019
@Squadrick Squadrick deleted the rename-opt branch March 29, 2019 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seanpmorgan seanpmorgan seanpmorgan approved these changes

+1 more reviewer

@armando-fandango armando-fandango armando-fandango left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@facaiy facaiy

Labels

cla: yes optimizers

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Squadrick @facaiy @armando-fandango @seanpmorgan @googlebot @kokoro-team