Skip to content

Add AdamW to C++ frontend #40009

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

Closed
wants to merge 9 commits into from
Closed

Add AdamW to C++ frontend #40009

wants to merge 9 commits into from

Conversation

sotlampr
Copy link
Contributor

@sotlampr sotlampr commented Jun 14, 2020

Slightly modified Adam, following the python implementation, and the ProducesPyTorchValues tests pass. I had a problem with another test though (see commit c1a6241), it seems that optimizing for two steps with the same optimizer vs optimizing for two steps using freshly initialized objects will produce the same output.

@dr-ci
Copy link

dr-ci bot commented Jun 14, 2020

💊 CI failures summary and remediations

As of commit 3ea1f90 (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 15 times.

@yf225 yf225 requested a review from glaringlee June 15, 2020 14:25
@glaringlee
Copy link
Contributor

@sotlampr
It seems this PR also included other people's changes. Can you rebase your branch to include your change only? i will review this ASAP. thx

@sotlampr
Copy link
Contributor Author

Yeah, I rebased to pass some broken tests. Should be good now. Thanks!

Copy link
Contributor

@glaringlee glaringlee left a comment

Choose a reason for hiding this comment

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

This is good to go now.
@sotlampr
Thank you so much for doing this work !

@sotlampr
Copy link
Contributor Author

Awesome, thanks @glaringlee

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@glaringlee has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@glaringlee merged this pull request in 41f2dbd.

xwang233 pushed a commit to xwang233/pytorch that referenced this pull request Jun 20, 2020
Summary:
Slightly modified Adam, following the python implementation, and the `ProducesPyTorchValues` tests pass. I had a problem with another test though (see commit c1a6241), it seems that optimizing for two steps with the same optimizer vs optimizing for two steps using freshly initialized objects will produce the same output.
Pull Request resolved: pytorch#40009

Differential Revision: D22096053

Pulled By: glaringlee

fbshipit-source-id: a31a8f5488cb37c53752ddf15436efabdba67dc4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants