Conversation
facebook-github-bot
added
oncall: jit
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit f4924e2 (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).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
Codecov Report
@@ Coverage Diff @@
## master #64257 +/- ##
=======================================
Coverage 66.37% 66.38%
=======================================
Files 739 739
Lines 94299 94310 +11
=======================================
+ Hits 62595 62608 +13
+ Misses 31704 31702 -2 |
|
Gentle ping @mruberry @saketh-are |
saketh-are
left a comment
saketh-are
left a comment
There was a problem hiding this comment.
Thanks for implementing this @kshitij12345. I left a minor comment regarding the documentation of argwhere's output format.
torch/_torch_docs.py
Outdated
| r""" | ||
| argwhere(input) -> LongTensor | ||
|
|
||
| Return the indices of array elements that are non-zero, grouped by element. |
There was a problem hiding this comment.
I found the "grouped by element" language from numpy's documentation to be pretty informal and hard to understand. Looks like I'm also not the first one: https://stackoverflow.com/questions/52400354/how-to-understand-the-np-argwhere-function/52400485.
It seems that "grouped by element" is stated in numpy.argwhere's documentation in reference to numpy.nonzero's behavior (https://numpy.org/doc/stable/reference/generated/numpy.nonzero.html), which groups by dimension instead.
For our purposes it seems clearer to reuse/refer to the language from torch.nonzero's documentation of its as_tuple=False case: https://pytorch.org/docs/stable/generated/torch.nonzero.html.
There was a problem hiding this comment.
That makes sense. Thanks!
|
@saketh-are should be ready for another review. Thanks :) |
saketh-are
left a comment
saketh-are
left a comment
There was a problem hiding this comment.
Hey @kshitij12345, thanks for making those changes! I wanted to confirm one last thing with you before we move on to landing this. Given that this PR relies on torch.nonzeros's correctness, would you be able to review the completeness of torch.nonzero's forward tests and make a small comment here about them? I also left a few minor comments inline.
mruberry
left a comment
mruberry
left a comment
There was a problem hiding this comment.
This looks really good @kshitij12345, I just made a few small comments inline for your review. I'll let @saketh-are merge this when it's ready.
Looking forward, we should consider deprecating nonzero in favor of argwhere. nonzero is popular so this will likely count as a "difficult deprecation," and we should prepare an RFC including it and other difficult deprecations (like max and min) soon to gauge the community's interest in NumPy consistency vs. disruptions.
CI Flow Status⚛️ CI FlowRuleset - Version:
You can add a comment to the PR and tag @pytorchbot with the following commands: # ciflow rerun, "ciflow/default" will always be added automatically
@pytorchbot ciflow rerun
# ciflow rerun with additional labels "-l <ciflow/label_name>", which is equivalent to adding these labels manually and trigger the rerun
@pytorchbot ciflow rerun -l ciflow/scheduled -l ciflow/slowFor more information, please take a look at the CI Flow Wiki. |
|
@saketh-are has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@saketh-are gentle ping :) |
|
@saketh-are has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@saketh-are merged this pull request in 462f333. |
|
Reopening due to f29e522 |
|
Failures look to be about |
|
The error was fixed in #67039 @saketh-are PTAL :) |
|
@saketh-are has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
5 participants
Adds
torch.argwhereas an alias totorch.nonzeroCurrently,
torch.nonzerois actually provides equivalent functionality tonp.argwhere.From NumPy docs,