numpy like nonzero (called nonzero_tuple) #20293

umanwizard · 2019-05-09T01:11:17Z

No performance degradation compared to Numpy when indexing:

In [15]: x=torch.randn((1000,1000))                                                                                                                                                         

In [16]: %timeit x[x.nonzero_tuple()]                                                                                                                                                       
4.63 ms ± 102 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

In [17]: y=x.numpy()                                                                                                                                                                        

In [18]: %timeit y[y.nonzero()]                                                                                                                                                             
14.6 ms ± 281 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

In [20]: x=x.t()                                                                                                                                                                            

In [22]: %timeit x[x.nonzero_tuple()]                                                                                                                                                       
9.01 ms ± 626 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

In [24]: y=x.numpy()                                                                                                                                                                        

In [25]: %timeit y[y.nonzero()]                                                                                                                                                             
16.8 ms ± 770 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

torch/_torch_docs.py

gchanan · 2019-05-09T14:49:16Z

that benchmark seems good, but wouldn't I also want to know the straight up runtime of the old-vs-the-new; you don't always index after a nonzero.

umanwizard · 2019-05-09T15:43:31Z

@gchanan it should be essentially the same since unbind is free (modulo overhead)

In [4]: %timeit x.nonzero()                                                                                                                                                                                                                   
3.72 ms ± 113 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

In [6]: %timeit x.nonzero_tuple()                                                                                                                                                                                                             
3.67 ms ± 87.3 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

colesbury

To echo @ssnl's point: a kwarg is consistent with what we've done in the past, discussed in #2739, and with NumPy's unique function.

test/test_torch.py

facebook-github-bot

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

gchanan · 2019-05-15T23:50:48Z

torch/functional.py

What do you think about doing all the parsing at the python arg parser level? That way we are guaranteed to treat everything consistently.

Right now there is no support there afaik for returning different result types depending on the value of a kwarg, so it would have to be significantly refactored.

can't you just handwrite the parsing code? At the end of the day, the parsing related code just returns a PyObject, so this should be fine, no?

gchanan

you seem to have some third_party changes, that shouldn't be in here.

torch/tensor.py

aten/src/ATen/native/Indexing.cpp

aten/src/ATen/native/native_functions.yaml

tools/pyi/gen_pyi.py

torch/_tensor_docs.py

torch/_torch_docs.py

torch/functional.py

torch/tensor.py

torch/_torch_docs.py

gchanan

There's a couple of unresolved comments here, but overall looks good.

facebook-github-bot

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

Summary: No performance degradation compared to Numpy when indexing: ``` In [15]: x=torch.randn((1000,1000)) In [16]: %timeit x[x.nonzero_tuple()] 4.63 ms ± 102 µs per loop (mean ± std. dev. of 7 runs, 100 loops each) In [17]: y=x.numpy() In [18]: %timeit y[y.nonzero()] 14.6 ms ± 281 µs per loop (mean ± std. dev. of 7 runs, 100 loops each) In [20]: x=x.t() In [22]: %timeit x[x.nonzero_tuple()] 9.01 ms ± 626 µs per loop (mean ± std. dev. of 7 runs, 100 loops each) In [24]: y=x.numpy() In [25]: %timeit y[y.nonzero()] 16.8 ms ± 770 µs per loop (mean ± std. dev. of 7 runs, 100 loops each) ``` Pull Request resolved: pytorch/pytorch#20293 Differential Revision: D15358754 Pulled By: umanwizard fbshipit-source-id: 1344aabd95c969eeda9780c475a39551231879e1

facebook-github-bot · 2019-06-07T02:36:01Z

@umanwizard merged this pull request in f4f32ce.

umanwizard requested a review from gchanan May 9, 2019 01:11

pytorchbot added module: docs Related to our documentation, both in docs/ and docblocks module: internals Related to internal abstractions in c10 and ATen module: operators labels May 9, 2019

umanwizard added module: numpy Related to numpy support, and also numpy compatibility of our operators and removed module: docs Related to our documentation, both in docs/ and docblocks module: internals Related to internal abstractions in c10 and ATen labels May 9, 2019

ssnl reviewed May 9, 2019

View reviewed changes

torch/_torch_docs.py Outdated Show resolved Hide resolved

colesbury reviewed May 9, 2019

View reviewed changes

test/test_torch.py Outdated Show resolved Hide resolved

pytorchbot added module: autograd Related to torch.autograd, and the autograd engine in general module: docs Related to our documentation, both in docs/ and docblocks module: internals Related to internal abstractions in c10 and ATen module: third_party labels May 10, 2019

umanwizard requested review from colesbury and fmassa May 13, 2019 20:23

umanwizard mentioned this pull request May 13, 2019

nonzero doesn't squeeze dimension #1834

Closed

pytorchbot added the module: typing Related to mypy type annotations label May 14, 2019

facebook-github-bot reviewed May 15, 2019

View reviewed changes

gchanan reviewed May 15, 2019

View reviewed changes

torch/tensor.py Outdated Show resolved Hide resolved

umanwizard force-pushed the np_nonzero branch from 031a012 to a85dec8 Compare May 16, 2019 18:03

umanwizard requested a review from gchanan May 16, 2019 18:04

gchanan requested changes May 16, 2019

View reviewed changes

pytorchbot added the module: pybind Related to our Python bindings / interactions with other Python libraries label May 20, 2019

umanwizard requested a review from gchanan May 20, 2019 18:59

gchanan reviewed May 21, 2019

View reviewed changes

torch/_torch_docs.py Outdated Show resolved Hide resolved

umanwizard requested a review from gchanan May 22, 2019 18:03

Brennan Vincent added 19 commits May 31, 2019 13:56

make requested changes

403b46c

remove unused stuff

ba7b72d

make zero-dim case behave like numpy

97034b0

python formatting

efe49e9

fix docs

e6d12e8

fix example

8a46ffc

blacklist nonzero from stub generation

fb02842

revert

ba86517

dont accept as_tuple as positional arg

d92e6ae

wip

4760b33

move parsing to cpp

71be520

fix docs

cb778f4

remove whitespace change

bce3186

s/nonzero_tuple/nonzero_numpy/g

66700d0

oops

2cb6fd5

change to docs

496abf0

asdf

5df1380

attempt to fix mypy issue

73e4aac

fix test on cuda

99e5159

umanwizard force-pushed the np_nonzero branch from 4588273 to 99e5159 Compare May 31, 2019 18:07

gchanan approved these changes Jun 4, 2019

View reviewed changes

code review

bcef635

facebook-github-bot reviewed Jun 4, 2019

View reviewed changes

facebook-github-bot closed this in f4f32ce Jun 6, 2019

facebook-github-bot added the merged label Jun 7, 2019

ssnl mentioned this pull request Jul 26, 2019

Allowing batching for det/logdet/slogdet operations #22909

Closed

mruberry added the Merged label Oct 28, 2020

numpy like nonzero (called nonzero_tuple) #20293

numpy like nonzero (called nonzero_tuple) #20293

Uh oh!

Conversation

umanwizard commented May 9, 2019

Uh oh!

Uh oh!

gchanan commented May 9, 2019

Uh oh!

umanwizard commented May 9, 2019

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

gchanan May 15, 2019

Choose a reason for hiding this comment

Uh oh!

umanwizard May 16, 2019

Choose a reason for hiding this comment

Uh oh!

gchanan May 16, 2019

Choose a reason for hiding this comment

Uh oh!

gchanan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gchanan left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jun 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants