Skip to content

Clarify and correct the doc of atan2. #26180

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

Conversation

xuhdev
Copy link
Collaborator

@xuhdev xuhdev commented Sep 13, 2019

No description provided.

@pytorchbot pytorchbot added the module: docs Related to our documentation, both in docs/ and docblocks label Sep 13, 2019
@xuhdev xuhdev requested review from gchanan, yf225 and izdeby and removed request for gchanan and yf225 September 13, 2019 18:48
@xuhdev
Copy link
Collaborator Author

xuhdev commented Sep 16, 2019

@pytorchbot rebase this please

@gchanan
Copy link
Contributor

gchanan commented Sep 17, 2019

do you know why these tests are failing?

@xuhdev xuhdev force-pushed the atan2-doc branch 2 times, most recently from 88b236f to 29334f8 Compare September 17, 2019 19:12
@xuhdev
Copy link
Collaborator Author

xuhdev commented Sep 17, 2019

@gchanan The reason the test failed before was {i}, which should be {{i}} to avoid an error in a format call.

@xuhdev xuhdev requested a review from gchanan September 17, 2019 19:14
@gchanan gchanan requested a review from albanD September 17, 2019 22:04
Returns a new tensor with the arctangent of the elements of :attr:`input`
and :attr:`other`.
Returns a new tensor with the arctangent of the angle between
:math:`(\text{{other}}_{{i}}, \text{{input}}_{{i}})` and :math:`(1, 0)`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you talk about vectors here, it should be be (input[i], other[i]) no?
Or you can define it as tan^{-1} ( other[i] / input[i]).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually maybe something inspired from what the numpy doc says would be clearer?

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 rewrote this paragraph. Please see the new update

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.

Minor improvements in the text, but otherwise fine.
Thanks for the PR !

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.

Thanks for that !

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.

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

@facebook-github-bot
Copy link
Contributor

@albanD merged this pull request in 71ec9a0.

mingbowan pushed a commit to mingbowan/pytorch that referenced this pull request Sep 23, 2019
Summary: Pull Request resolved: pytorch#26180

Reviewed By: ezyang

Differential Revision: D17500224

Pulled By: albanD

fbshipit-source-id: 98b9f32aa443963fe1e89b83e15bed9ff83a2694
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merged module: docs Related to our documentation, both in docs/ and docblocks open source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants