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

Complex autograd doc fix #46258

Closed
wants to merge 3 commits into from
Closed

Conversation

anjali411
Copy link
Contributor

@anjali411 anjali411 commented Oct 13, 2020

Stack from ghstack:

Differential Revision: D24286512

[ghstack-poisoned]
@anjali411 anjali411 added this to the 1.7.0 milestone Oct 13, 2020
@@ -222,7 +222,7 @@ The short version:
the gradients are computed under the assumption that the function is a part of a larger real-valued
loss function :math:`g(input)=L`. The gradient computed is :math:`\frac{\partial L}{\partial z^*}`
(note the conjugation of z), which is precisely the direction of the step
you should take in gradient descent. Thus, all the existing optimizers work out of
you should take in gradient ascent. Thus, all the existing optimizers work out of
Copy link
Collaborator

Choose a reason for hiding this comment

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

While it is true, I don't think anyone is doing gradient ascent anymore :D
I think that not specifying is better here: "which is precisely the direction expected by gradient-based optimization algorithms." What do you think?

Copy link
Contributor Author

@anjali411 anjali411 Oct 13, 2020

Choose a reason for hiding this comment

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

yeah that sounds good! alternately we can also consider:
" ... (note the conjugation of z), the negative of which is precisely the direction of steepest descent in Gradient Descent algorithm." it might be useful to make reference to Gradient Descent since (as you mentioned) practically majority of people use Gradient Descent as opposed to other alternatives that fall under "gradient-based optimization algorithms" ...

@dr-ci
Copy link

dr-ci bot commented Oct 13, 2020

💊 CI failures summary and remediations

As of commit 512668c (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 8 times.

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.

lgtm

(note the conjugation of z), which is precisely the direction of the step
you should take in gradient descent. Thus, all the existing optimizers work out of
(note the conjugation of z), the negative of which is precisely the direction of steepest descent
in Gradient Descent algorithm.. Thus, all the existing optimizers work out of
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: missing a "used"

anjali411 added a commit that referenced this pull request Oct 13, 2020
ghstack-source-id: 03ba913f8b673163b5c4e9456c7eff3529eb237b
Pull Request resolved: #46258
(note the conjugation of z), which is precisely the direction of the step
you should take in gradient descent. Thus, all the existing optimizers work out of
(note the conjugation of z), the negative of which is precisely the direction of steepest descent
used in Gradient Descent algorithm.. Thus, all the existing optimizers work out of
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: two periods

@facebook-github-bot
Copy link
Contributor

@anjali411 merged this pull request in ac245f6.

@facebook-github-bot facebook-github-bot deleted the gh/anjali411/64/head branch October 17, 2020 14:15
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.

None yet

4 participants