[doc] correct docs for `torch.nonzero` by stas00 · Pull Request #43434 · pytorch/pytorch

stas00 · 2020-08-21T23:02:58Z

As discussed at #43425

as_tuple is a required argument since pytorch-1.5. If not passed, a warning will be issued.
adjust the examples to reflect that
fix the function signature to reflect reality (out out, as_tuple in)

As discussed at pytorch#43425 - `as_tuple` is a required argument since pytorch-1.5. If not passed a warning will be issued. - adjust the docs - add the missing 3nd arg in Args

albanD · 2020-08-21T23:13:16Z

torch/_torch_docs.py

    gives all nonzero values of tensor ``x``. Of the returned tuple, each index tensor
    contains nonzero indices for a certain dimension.

+    `as_tuple` is a required argument since pytorch-1.5. If not passed a warning


Is the default in the signature actually False? Isn't it None by any chance? Otherwise, how do we detect that the user did not provide it? :D

It is False, since it wasn't required until now.

That's why it's confusing, that it has a default, yet a user must pass the argument explicitly - perhaps it's a bug in the code?

Hmm, weird it has no default in the signature

torch/__init__.pyi:def nonzero(self: Tensor, *, out: Optional[Tensor]=None) -> Tensor: ..

Yep that's what I expected :D
If the default is actually False, there is no way the function can know if the user provided it or not.
I think you should change the signature in the doc to reflect that.

print(torch.nonzero(torch.tensor([1, 1, 1, 0, 1]))) print(torch.nonzero(torch.tensor([1, 1, 1, 0, 1]), as_tuple=False)) print(torch.nonzero(torch.tensor([1, 1, 1, 0, 1]), as_tuple=True))

the 1st and 2nd ones produce the same output (except the first one prints the warning)

Hmm, but as_tuple's default is not None.

I found where the ambiguity is flagged:

https://github.com/pytorch/pytorch/blob/master/tools/autograd/templates/python_torch_functions.cpp#L577
and when it's called on an object:
https://github.com/pytorch/pytorch/blob/master/tools/autograd/templates/python_variable_methods.cpp#L450

I don't seem to find the actual C++ function - too many unrelated nonzero strings. Do you by chance know where it is?

Without seeing the source code from everything I have seen so far, and if there is no better way to resolve it - the doc should say:

as_tuple is a required argument and that it has no default.

But should it still show the old signature? Separately? And if not, should out be removed from the doc?

see the further updates I made in subsequent commits.

I think I'll let @ssnl give his opinion here as he already did a doc update on this param in the past.

Thank you for your feedback so far, @albanD

updates based on discussion

dr-ci · 2020-08-22T00:59:34Z

💊 CI failures summary and remediations

As of commit 9159a04 (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:

pytorch_doc_test (1/2)

Step: "Doc test" (full log | diagnosis details | 🔁 rerun)

Aug 22 03:38:28 caused by: Connection refused (os error 111)

Aug 22 03:38:28 +++++ extract_trap_cmd 
Aug 22 03:38:28 +++++ printf '%s\n' '' 
Aug 22 03:38:28 ++++ printf '%s\n' cleanup 
Aug 22 03:38:28 +++ trap -- ' 
Aug 22 03:38:28 cleanup' EXIT 
Aug 22 03:38:28 +++ [[ pytorch-linux-xenial-py3.6-gcc5.4-build != *pytorch-win-* ]] 
Aug 22 03:38:28 +++ which sccache 
Aug 22 03:38:28 +++ sccache --stop-server 
Aug 22 03:38:28 Stopping sccache server... 
Aug 22 03:38:28 error: couldn't connect to server 
Aug 22 03:38:28 caused by: Connection refused (os error 111) 
Aug 22 03:38:28 +++ true 
Aug 22 03:38:28 +++ rm /var/lib/jenkins/sccache_error.log 
Aug 22 03:38:28 +++ SCCACHE_ERROR_LOG=/var/lib/jenkins/sccache_error.log 
Aug 22 03:38:28 +++ SCCACHE_IDLE_TIMEOUT=1200 
Aug 22 03:38:28 +++ RUST_LOG=sccache::server=error 
Aug 22 03:38:28 +++ sccache --start-server 
Aug 22 03:38:28 Starting sccache server... 
Aug 22 03:38:28 +++ sccache --zero-stats 
Aug 22 03:38:28 Compile requests                 0 
Aug 22 03:38:28 Compile requests executed        0

pytorch_python_doc_build (2/2)

Step: "Doc Build and Push" (full log | diagnosis details | 🔁 rerun)

Aug 22 03:49:30 Makefile:38: recipe for target 'html' failed

Aug 22 03:49:29 copying images... [100%] scripts/activation_images/ReLU6.png 
Aug 22 03:49:29  
Aug 22 03:49:29 copying static files... ... done 
Aug 22 03:49:29 copying extra files... done 
Aug 22 03:49:29 dumping search index in English (code: en)... done 
Aug 22 03:49:29 dumping object inventory... done 
Aug 22 03:49:29 build finished with problems, 2 warnings. 
Aug 22 03:49:29 /var/lib/jenkins/workspace/docs/src/pytorch-sphinx-theme/pytorch_sphinx_theme/search.html:21: RemovedInSphinx30Warning: To modify script_files in the theme is deprecated. Please insert a <script> tag directly in your theme instead. 
Aug 22 03:49:29   {% trans %}Please activate JavaScript to enable the search 
Aug 22 03:49:30 make: *** [html] Error 1 
Aug 22 03:49:30 Makefile:38: recipe for target 'html' failed

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 7 times.

stas00 · 2020-08-22T15:48:11Z

The CI failures appear unrelated to this PR

xkszltl · 2020-08-25T04:33:48Z

I think this is a bug of signature parser.
As I mentioned in comment, it is not a required flag.
#32994 (comment)
The call is routed to the wrong signature.
There should be a default value for this line as well:

pytorch/tools/autograd/templates/python_torch_functions.cpp

Line 582 in 675f3f0

"nonzero(Tensor input, *, bool as_tuple)",

To avoid ambiguity, maybe we should only deprecate nonzero(Tensor input, *, Tensor out), not the one with out=None.

stas00 · 2020-09-05T16:47:03Z

what needs to happen so that a decision is made to move towards a resolution? Thank you!

stas00 · 2020-09-07T19:27:18Z

Related issue: #44284

stas00 · 2020-12-19T01:15:46Z

Looks like another PR fixed the warning instead: #45413

ssnl · 2020-12-19T01:29:09Z

I just saw this PR from email @stas00 and @albanD . I am sorry that I wasn't able to promptly review this.

stas00 · 2020-12-19T01:33:05Z

Well, it proved to be unnecessary since the issue was fixed and the docs no longer need to be updated - so all is good.

At the very least I verified that the initial problem I reported here: #43425 has been fixed.

Thank you, @ssnl!

[doc] correct docs for torch.nonzero

7d4c521

As discussed at pytorch#43425 - `as_tuple` is a required argument since pytorch-1.5. If not passed a warning will be issued. - adjust the docs - add the missing 3nd arg in Args

stas00 mentioned this pull request Aug 21, 2020

nonzero() documentation examples issue a warning #43425

Closed

albanD reviewed Aug 21, 2020

View reviewed changes

pytorchbot added the open source label Aug 21, 2020

stas00 added 3 commits August 21, 2020 17:16

as_tuple is a required arg, remove out arg

7fe8eae

updates based on discussion

there are no extra args, remove *

fe89755

more clarification to why the weird requirements

311457a

try double quotes

9159a04

albanD requested a review from ssnl August 22, 2020 03:23

ngimel added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Aug 25, 2020

stas00 mentioned this pull request Sep 7, 2020

Current behavior of as_tuple argument is inconsistent in nonzero #44284

Closed

Jomonsugi mentioned this pull request Oct 20, 2020

torch.nonzero issues a DeprecationWarning for non-deprecated usages #32994

Closed

facebook-github-bot added the cla signed label Nov 10, 2020

stas00 closed this Dec 19, 2020

stas00 deleted the patch-1 branch December 19, 2020 01:15

Conversation

stas00 commented Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

albanD Aug 21, 2020

Choose a reason for hiding this comment

Uh oh!

stas00 Aug 21, 2020

Choose a reason for hiding this comment

Uh oh!

stas00 Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albanD Aug 21, 2020

Choose a reason for hiding this comment

Uh oh!

stas00 Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stas00 Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stas00 Aug 22, 2020

Choose a reason for hiding this comment

Uh oh!

stas00 Aug 22, 2020

Choose a reason for hiding this comment

Uh oh!

albanD Aug 22, 2020

Choose a reason for hiding this comment

Uh oh!

stas00 Aug 22, 2020

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

💊 CI failures summary and remediations

🕵️ 2 new failures recognized by patterns

pytorch_doc_test (1/2)

pytorch_python_doc_build (2/2)

Uh oh!

stas00 commented Aug 22, 2020

Uh oh!

xkszltl commented Aug 25, 2020

Uh oh!

stas00 commented Sep 5, 2020

Uh oh!

stas00 commented Sep 7, 2020

Uh oh!

stas00 commented Dec 19, 2020

Uh oh!

ssnl commented Dec 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stas00 commented Dec 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

stas00 commented Aug 21, 2020 •

edited

Loading

stas00 Aug 21, 2020 •

edited

Loading

stas00 Aug 21, 2020 •

edited

Loading

stas00 Aug 21, 2020 •

edited

Loading

dr-ci bot commented Aug 22, 2020 •

edited

Loading

ssnl commented Dec 19, 2020 •

edited

Loading