Skip to content

Fix boolean indexing.#8920

Closed
gchanan wants to merge 3 commits intopytorch:masterfrom
gchanan:boolean_indexing
Closed

Fix boolean indexing.#8920
gchanan wants to merge 3 commits intopytorch:masterfrom
gchanan:boolean_indexing

Conversation

@gchanan
Copy link
Contributor

@gchanan gchanan commented Jun 26, 2018

Booleaning indexing was special cased to handle a single boolean value, but didn't generally work given multiple booleans.
This PR unifies the behavior with slicing. Note that only 'True' and torch.tensor(True) behave like NumPy due to the lack of n-dimensional empty tensors.
The corresponding tests for false values have been added, but are guarded behind a flag until we add n-dimensional empty tensors.

@gchanan
Copy link
Contributor Author

gchanan commented Jun 26, 2018

Some simple examples of broken cases:

In [7]: a=torch.randn(2,3)

In [8]: a[True, True]
Out[8]: tensor(1.3917)

In [9]: true=torch.tensor(True)

In [10]: a[true, true]
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
<ipython-input-10-c74b16c4f1d6> in <module>()
----> 1 a[true, true]

RuntimeError: dim() called on undefined Tensor

@gchanan
Copy link
Contributor Author

gchanan commented Jun 26, 2018

Corresponding numpy behavior:

In [12]: b=np.random.randn(2,3)

In [13]: b[True, True].shape
Out[13]: (1, 2, 3)

In [14]: true=np.array(True)

In [15]: b[true, true].shape
Out[15]: (1, 2, 3)

Copy link
Member

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

sweet!

gchanan added 3 commits June 27, 2018 08:13
Booleaning indexing was special cased to handle a single boolean value, but didn't generally work given multiple booleans.
This PR unifies the behavior with slicing.  Note that only 'True' and torch.tensor(True) behave like NumPy due to the lack of n-dimensional empty tensors.
The corresponding tests for false values have been added, but are guarded behind a flag until we add n-dimensional empty tensors.
@gchanan gchanan force-pushed the boolean_indexing branch from 8257d70 to 2b8d75c Compare June 27, 2018 15:14
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.

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

@fmassa
Copy link
Member

fmassa commented Jun 27, 2018

BTW, does this fix #6773 ?

@gchanan
Copy link
Contributor Author

gchanan commented Jun 27, 2018

@fmassa doesn't appear to fix 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.

@ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@gchanan
Copy link
Contributor Author

gchanan commented Jul 3, 2018

@pytorchbot retest this please.

goodlux pushed a commit to goodlux/pytorch that referenced this pull request Aug 15, 2018
Summary:
Booleaning indexing was special cased to handle a single boolean value, but didn't generally work given multiple booleans.
This PR unifies the behavior with slicing.  Note that only 'True' and torch.tensor(True) behave like NumPy due to the lack of n-dimensional empty tensors.
The corresponding tests for false values have been added, but are guarded behind a flag until we add n-dimensional empty tensors.
Closes pytorch#8920

Reviewed By: ezyang

Differential Revision: D8661876

Pulled By: gchanan

fbshipit-source-id: 0dc8a45a303aa41f729d04ab8908cfaf2e3ce3d7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants