KEMBAR78
To add Nesterov Adam algorithm description to documentation by iramazanli · Pull Request #63793 · pytorch/pytorch · GitHub
Skip to content

To add Nesterov Adam algorithm description to documentation #63793

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
Closed
wants to merge 1 commit into from
Closed

To add Nesterov Adam algorithm description to documentation #63793

wants to merge 1 commit into from

Conversation

@iramazanli
Copy link
Contributor

@iramazanli iramazanli commented Aug 23, 2021 •
edited
Loading

It has been discussed before that adding description of Optimization algorithms to PyTorch Core documentation may result in a nice Optimization research tutorial. In the following tracking issue we mentioned about all the necessary algorithms and links to the originally published paper #63236.

In this PR we are adding description of Nesterov Adam Algorithm to the documentation. For more details, we refer to the paper https://openreview.net/forum?id=OM0jvwB8jIp57ZJjtNEZ

NAdam

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Aug 23, 2021 •
edited
Loading

🔗 Helpful links

💊 CI failures summary and remediations

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

  • 2/2 failures introduced in this PR

🕵️ 2 new failures recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

See CircleCI build pytorch_xla_linux_bionic_py3_6_clang9_build (1/2)

Step: "(Optional) Merge target branch" (full log | diagnosis details | 🔁 rerun)

Automatic merge failed; fix conflicts and then commit the result. 
CONFLICT (add/add): Merge conflict in .circleci/docker/common/install_cmake.sh
Auto-merging .circleci/docker/common/install_cmake.sh
CONFLICT (add/add): Merge conflict in .circleci/docker/build.sh
Auto-merging .circleci/docker/build.sh
CONFLICT (add/add): Merge conflict in .circleci/config.yml
Auto-merging .circleci/config.yml
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/pytorch_build_definitions.py
Auto-merging .circleci/cimodel/data/pytorch_build_definitions.py
CONFLICT (add/add): Merge conflict in .bazelrc
Auto-merging .bazelrc
Automatic merge failed; fix conflicts and then commit the result.


Exited with code exit status 1

See CircleCI build pytorch_linux_xenial_py3_6_gcc5_4_build (2/2)

Step: "(Optional) Merge target branch" (full log | diagnosis details | 🔁 rerun)

Automatic merge failed; fix conflicts and then commit the result. 
CONFLICT (add/add): Merge conflict in .circleci/docker/common/install_cmake.sh
Auto-merging .circleci/docker/common/install_cmake.sh
CONFLICT (add/add): Merge conflict in .circleci/docker/build.sh
Auto-merging .circleci/docker/build.sh
CONFLICT (add/add): Merge conflict in .circleci/config.yml
Auto-merging .circleci/config.yml
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/pytorch_build_definitions.py
Auto-merging .circleci/cimodel/data/pytorch_build_definitions.py
CONFLICT (add/add): Merge conflict in .bazelrc
Auto-merging .bazelrc
Automatic merge failed; fix conflicts and then commit the result.


Exited with code exit status 1
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 to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@iramazanli iramazanli force-pushed the nadam_algorithm_doc branch 2 times, most recently from c6068f0 to 84a21b6 Compare August 24, 2021 19:07
@codecov
Copy link

codecov bot commented Aug 24, 2021 •
edited
Loading

Codecov Report

Merging #63793 (84a21b6) into master (f0d2742) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 84a21b6 differs from pull request most recent head 96b0de5. Consider uploading reports for the commit 96b0de5 to get more accurate results

@@            Coverage Diff             @@
##           master   #63793      +/-   ##
==========================================
- Coverage   67.09%   67.07%   -0.02%     
==========================================
  Files         692      691       -1     
  Lines       90579    90571       -8     
==========================================
- Hits        60774    60753      -21     
- Misses      29805    29818      +13

@iramazanli iramazanli force-pushed the nadam_algorithm_doc branch 2 times, most recently from 7071236 to 96b0de5 Compare August 26, 2021 22:59
@iramazanli iramazanli changed the title To add Nesterov Adam algorithm To add Nesterov Adam algorithm description to documentation Aug 27, 2021
@iramazanli iramazanli force-pushed the nadam_algorithm_doc branch 2 times, most recently from e18aff0 to 37be604 Compare August 27, 2021 18:53
@iramazanli iramazanli requested a review from albanD August 27, 2021 18:54
@iramazanli iramazanli force-pushed the nadam_algorithm_doc branch 3 times, most recently from b949b7b to 217c910 Compare August 27, 2021 20:11
albanD
albanD reviewed Aug 27, 2021
View reviewed changes
torch/optim/nadam.py Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

t initialization is not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed :)

torch/optim/nadam.py Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would do t \psi so that it cannot be confused with \psi_t which is very similar when things are small.

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: factor out \beta_1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good :)

torch/optim/nadam.py Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: it is kind of an abuse of notation to use \mu_{t+1} here :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, i agree, so i added one more line that is reflected in rendered version

@iramazanli iramazanli force-pushed the nadam_algorithm_doc branch from 217c910 to c0d4bfa Compare August 27, 2021 20:23
@iramazanli iramazanli force-pushed the nadam_algorithm_doc branch from c0d4bfa to 8164076 Compare August 27, 2021 20:31
albanD
albanD approved these changes Aug 27, 2021
View reviewed changes
Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

ok

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@iramazanli merged this pull request in 9ccb929.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@albanD albanD albanD approved these changes

Assignees

No one assigned

Labels

cla signed Merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@iramazanli @facebook-github-bot @albanD