Add torch.logit function #41062

xiaomengy · 2020-07-07T04:33:24Z

Summary: Add torch.logit function

Test Plan: buck test mode/dev-nosan //caffe2/test:torch -- "logit"

Differential Revision: D22406912

facebook-github-bot · 2020-07-07T04:33:42Z

This pull request was exported from Phabricator. Differential Revision: D22406912

xiaomengy · 2020-07-07T04:34:16Z

link #37060

mruberry · 2020-07-07T05:01:12Z

Thanks for putting a PR up, @BIT-silence. I'll get you a review ASAP. Want to add a couple helpful links now before I lose them:

SciPy's logit function: https://docs.scipy.org/doc/scipy/reference/generated/scipy.special.logit.html
SciPy implementation: https://github.com/scipy/scipy/blob/8dba340293fe20e62e173bdf2c10ae208286692f/scipy/special/_logit.c.src#L23.

dr-ci · 2020-07-07T06:47:07Z

💊 CI failures summary and remediations

As of commit a2dc859 (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 102 times.

ngimel

Small comment about type of scalar arg

aten/src/ATen/native/cuda/BinaryMiscBackwardOpsKernels.cu

mruberry

Hey @BIT-silence! What's here looks pretty good, but there are a few more things to do:

add a doc entry in docs/source/tensors.rst and docs/source/torch.rst
add the actual docs in torch/_tensor_docs.py and torch/_torch_docs.py
test the backward works as expected by adding an entry to torch/testing/_internal/common_method_invocations.py
add an entry in torch/_overrides.py so people can override this function when using the torch_function interface

Also, what do you think about adding an explicit test for extremal values in test_torch? Like -inf, -5, 0, 1, 2, inf, NaN?

aten/src/ATen/native/native_functions.yaml

ngimel · 2020-07-08T17:43:49Z

Also, what do you think about adding an explicit test for extremal values in test_torch? Like -inf, -5, 0, 1, 2, inf, NaN?

Instead of adding separate tests for extremal values, perhaps it makes sense to add those to TorchMathTest so that all functions there are tests? It already has "interesting" finite values, but it indeed misses infs and nans.

facebook-github-bot · 2020-07-09T01:24:35Z

This pull request was exported from Phabricator. Differential Revision: D22406912

facebook-github-bot · 2020-07-09T03:02:57Z

This pull request was exported from Phabricator. Differential Revision: D22406912

facebook-github-bot · 2020-07-09T05:45:57Z

This pull request was exported from Phabricator. Differential Revision: D22406912

facebook-github-bot · 2020-07-09T19:14:58Z

This pull request was exported from Phabricator. Differential Revision: D22406912

xiaomengy · 2020-07-09T19:26:43Z

Also, what do you think about adding an explicit test for extremal values in test_torch? Like -inf, -5, 0, 1, 2, inf, NaN?

Instead of adding separate tests for extremal values, perhaps it makes sense to add those to TorchMathTest so that all functions there are tests? It already has "interesting" finite values, but it indeed misses infs and nans.

I think we can consider open another issue for this. I tried to add inf and nan in the current tests, but some of the existing test will fail because of the different behavior for outputing inf or nan. If we want to make everything match with numpy/scipy, maybe some more works are needed.

mruberry · 2020-07-09T23:10:53Z

Also, what do you think about adding an explicit test for extremal values in test_torch? Like -inf, -5, 0, 1, 2, inf, NaN?

Instead of adding separate tests for extremal values, perhaps it makes sense to add those to TorchMathTest so that all functions there are tests? It already has "interesting" finite values, but it indeed misses infs and nans.

I think we can consider open another issue for this. I tried to add inf and nan in the current tests, but some of the existing test will fail because of the different behavior for outputing inf or nan. If we want to make everything match with numpy/scipy, maybe some more works are needed.

Can you elaborate on this? It's important we be compatible with NumPy or we may have to go through a painful deprecation process later. We already test several torch functions against NumPy for extremal values like this and comparing their values directly hasn't been a problem so far.

facebook-github-bot · 2020-07-12T10:57:04Z

This pull request was exported from Phabricator. Differential Revision: D22406912

facebook-github-bot · 2020-07-12T21:54:15Z

This pull request was exported from Phabricator. Differential Revision: D22406912

facebook-github-bot · 2020-07-13T00:07:47Z

This pull request was exported from Phabricator. Differential Revision: D22406912

facebook-github-bot · 2020-07-13T03:42:34Z

This pull request was exported from Phabricator. Differential Revision: D22406912

facebook-github-bot · 2020-07-13T17:35:40Z

This pull request was exported from Phabricator. Differential Revision: D22406912

ngimel

Looks good now, modulo a small documentation fix and a question about symbolic gradient.

torch/_torch_docs.py

ngimel · 2020-07-13T18:20:58Z

torch/csrc/jit/runtime/symbolic_script.cpp

why are you defining symbolic gradient? This would cause scripted module run this implementation instead of built-in backward, and without fuser this will be much slower. And I'm not sure if fuser (which of them?) is able to efficiently fuse this backward.

I don't know why if I didn't add it here, there will be an error that autodiff node number didn't match. So I added it here to test that.

Which test was failing?

facebook-github-bot · 2020-07-13T19:07:13Z

This pull request was exported from Phabricator. Differential Revision: D22406912

mruberry

Cool! Thanks @BIT-silence! Really appreciate you taking the time to make the edge case behavior consistent and to validate it!

Summary: Pull Request resolved: pytorch#41062 Add torch.logit function Test Plan: buck test mode/dev-nosan //caffe2/test:torch -- "logit" Differential Revision: D22406912 fbshipit-source-id: 0c66ce78b2ae82dfbe39a221f857c0330a7acb86

facebook-github-bot · 2020-07-13T22:01:21Z

This pull request was exported from Phabricator. Differential Revision: D22406912

facebook-github-bot · 2020-07-14T04:12:19Z

This pull request has been merged in 80d5b37.

facebook-github-bot added the fb-exported label Jul 7, 2020

xiaomengy requested a review from ngimel July 7, 2020 04:34

xiaomengy mentioned this pull request Jul 7, 2020

Add Logit function #37060

Closed

mruberry added the module: numpy Related to numpy support, and also numpy compatibility of our operators label Jul 7, 2020

mruberry self-requested a review July 7, 2020 04:56

ngimel approved these changes Jul 8, 2020

View reviewed changes

aten/src/ATen/native/cuda/BinaryMiscBackwardOpsKernels.cu Outdated Show resolved Hide resolved

mruberry requested changes Jul 8, 2020

View reviewed changes

gchanan reviewed Jul 8, 2020

View reviewed changes

aten/src/ATen/native/native_functions.yaml Outdated Show resolved Hide resolved

xiaomengy force-pushed the export-D22406912 branch from a1b0368 to a456d79 Compare July 9, 2020 01:24

xiaomengy force-pushed the export-D22406912 branch from a456d79 to 02362f4 Compare July 9, 2020 03:02

xiaomengy force-pushed the export-D22406912 branch from 02362f4 to c995b72 Compare July 9, 2020 05:45

xiaomengy force-pushed the export-D22406912 branch from c995b72 to 232f0bd Compare July 9, 2020 19:15

xiaomengy requested a review from mruberry July 9, 2020 19:28

xiaomengy force-pushed the export-D22406912 branch from 232f0bd to 6b46499 Compare July 9, 2020 23:27

xiaomengy force-pushed the export-D22406912 branch from 0c7485d to 3b3c865 Compare July 12, 2020 10:57

xiaomengy force-pushed the export-D22406912 branch from 3b3c865 to 6967a9c Compare July 12, 2020 21:54

xiaomengy requested a review from apaszke as a code owner July 12, 2020 21:54

xiaomengy force-pushed the export-D22406912 branch from 6967a9c to 69bfe1a Compare July 13, 2020 00:07

xiaomengy force-pushed the export-D22406912 branch from 69bfe1a to 09692ce Compare July 13, 2020 03:42

xiaomengy force-pushed the export-D22406912 branch from 09692ce to 7da7494 Compare July 13, 2020 17:35

ngimel approved these changes Jul 13, 2020

View reviewed changes

xiaomengy force-pushed the export-D22406912 branch from 7da7494 to 289417f Compare July 13, 2020 19:07

mruberry approved these changes Jul 13, 2020

View reviewed changes

Add torch.logit function (pytorch#41062)

a2dc859

Summary: Pull Request resolved: pytorch#41062 Add torch.logit function Test Plan: buck test mode/dev-nosan //caffe2/test:torch -- "logit" Differential Revision: D22406912 fbshipit-source-id: 0c66ce78b2ae82dfbe39a221f857c0330a7acb86

xiaomengy force-pushed the export-D22406912 branch from 289417f to a2dc859 Compare July 13, 2020 22:01

facebook-github-bot closed this in 80d5b37 Jul 14, 2020

facebook-github-bot added the merged label Jul 14, 2020

xiaomengy deleted the export-D22406912 branch July 14, 2020 04:22

mruberry added the Merged label Oct 28, 2020

Add torch.logit function #41062

Add torch.logit function #41062

Uh oh!

Conversation

xiaomengy commented Jul 7, 2020

Uh oh!

facebook-github-bot commented Jul 7, 2020

Uh oh!

xiaomengy commented Jul 7, 2020

Uh oh!

mruberry commented Jul 7, 2020

Uh oh!

dr-ci bot commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

ngimel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mruberry left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ngimel commented Jul 8, 2020

Uh oh!

facebook-github-bot commented Jul 9, 2020

Uh oh!

facebook-github-bot commented Jul 9, 2020

Uh oh!

facebook-github-bot commented Jul 9, 2020

Uh oh!

facebook-github-bot commented Jul 9, 2020

Uh oh!

xiaomengy commented Jul 9, 2020

Uh oh!

mruberry commented Jul 9, 2020

Uh oh!

facebook-github-bot commented Jul 12, 2020

Uh oh!

facebook-github-bot commented Jul 12, 2020

Uh oh!

facebook-github-bot commented Jul 13, 2020

Uh oh!

facebook-github-bot commented Jul 13, 2020

Uh oh!

facebook-github-bot commented Jul 13, 2020

Uh oh!

ngimel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ngimel Jul 13, 2020

Choose a reason for hiding this comment

Uh oh!

xiaomengy Jul 13, 2020

Choose a reason for hiding this comment

Uh oh!

ngimel Jul 13, 2020

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jul 13, 2020

Uh oh!

mruberry left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jul 13, 2020

Uh oh!

facebook-github-bot commented Jul 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dr-ci bot commented Jul 7, 2020 •

edited

Loading