Skip to content

Conversation

lezcano
Copy link
Collaborator

@lezcano lezcano commented Jun 1, 2022

Stack from ghstack:

I also took this chance to clean a bit the sphinx formatting and
reworded a few minor things.

I also took this chance to clean a bit the sphinx formatting and
reworded a few minor things.

[ghstack-poisoned]
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jun 1, 2022

🔗 Helpful links

✅ No Failures (0 Pending)

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

Expand to see more

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


This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@lezcano lezcano requested review from soulitzer and albanD June 1, 2022 13:18
@lezcano lezcano added module: docs Related to our documentation, both in docs/ and docblocks module: autograd Related to torch.autograd, and the autograd engine in general release notes: autograd release notes category labels Jun 1, 2022
lezcano added 4 commits June 1, 2022 14:24
I also took this chance to clean a bit the sphinx formatting and
reworded a few minor things.

[ghstack-poisoned]
I also took this chance to clean a bit the sphinx formatting and
reworded a few minor things.

[ghstack-poisoned]
I also took this chance to clean a bit the sphinx formatting and
reworded a few minor things.

[ghstack-poisoned]
I also took this chance to clean a bit the sphinx formatting and
reworded a few minor things.

[ghstack-poisoned]
Copy link
Contributor

@soulitzer soulitzer left a comment

Choose a reason for hiding this comment

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

Looks mostly good! Have some small comments. Thanks for fixing up the sphinx.

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.

Nice update!

#. If the function is concave (at least locally), use the super-gradient with minimum norm (using a similar argument as above).
#. If the function is defined, define the gradient at the current point by continuity (note that :math:`inf` is possible here, for example, :math:`sqrt(0)`). If multiple values are possible, pick one arbitrarily.
#. If the function is not defined (:math:`\sqrt(-1)`, :math:`\log(-1)` or most functions when the input is :math:`nan` for example) then the value used as the gradient is arbitrary (we might also raise an error but that is not guaranteed). Most functions will use :math:`nan` as the gradient, but for performance reasons, some functions will use non-:math:`nan` values (:math:`\log(-1)` for example).
#. If the function is convex (at least locally), use the sub-gradient of minimum norm (it the steepest descent direction).
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

Also why remove the reference?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought referencing an exercise in a random book was not too cool

if you think it's necessary, I can look up a proper reference, but also we never give references for anything else, so that reference looked a bit out of context

I also took this chance to clean a bit the sphinx formatting and
reworded a few minor things.

[ghstack-poisoned]
@lezcano
Copy link
Collaborator Author

lezcano commented Jun 7, 2022

@albanD this is ready for another review

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.

SGTM

@lezcano
Copy link
Collaborator Author

lezcano commented Jun 8, 2022

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a merge job. Check the current status here

@github-actions
Copy link
Contributor

github-actions bot commented Jun 8, 2022

Hey @lezcano.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

facebook-github-bot pushed a commit that referenced this pull request Jun 10, 2022
Summary:
I also took this chance to clean a bit the sphinx formatting and
reworded a few minor things.

Pull Request resolved: #78617

Approved by: https://github.com/soulitzer, https://github.com/albanD

Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/a8ea58afeeb4aed902e52478384003505fbbb107

Reviewed By: osalpekar

Differential Revision: D37025684

Pulled By: osalpekar

fbshipit-source-id: dcbb76366365e05a9c83d70820a531d4c6cc4b3a
@facebook-github-bot facebook-github-bot deleted the gh/Lezcano/86/head branch June 12, 2022 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla signed Merged module: autograd Related to torch.autograd, and the autograd engine in general module: docs Related to our documentation, both in docs/ and docblocks open source release notes: autograd release notes category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants