Skip to content

BUG: Raise correct errors in boolean indexing fast path #17010

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Aug 6, 2020

Conversation

asmeurer
Copy link
Member

@asmeurer asmeurer commented Aug 4, 2020

Previously the logic assumed that an index was valid if the size was the same
as the indexed array, rather than the shape. This resulted in an index being
incorrectly allowed if the index was all False, like np.empty((3, 3))[np.full((1, 9),
False)], or incorrectly giving a ValueError about broadcasting if the index
was not all False (like np.empty((3, 3))[np.full((1, 9), True)]). Now these
examples both give IndexError with the usual error message.

Fixes #16997.

(not really sure about the NumPy commit message flags, or which one should apply to this. Is this considered an API break?)

Previously the logic assumed that an index was valid if the size was the same
as the indexed array, rather than the shape. This resulted in an index being
incorrectly allowed if the index was all False, like np.empty((3, 3))[np.full((1, 9),
False)], or incorrectly giving a ValueError about broadcasting if the index
was not all False (like np.empty((3, 3))[np.full((1, 9), True)]). Now these
examples both give IndexError with the usual error message.

Fixes numpy#16997.
@seberg
Copy link
Member

seberg commented Aug 5, 2020

Probably wouldn't use API, considering how small the change is. But a compatibility release note is necessary, at least for the wrong result → error change. The comment should now be changed to say that the shape check is always an error, and only there to ensure the same error message as in the generic path (the generic path sets it a bit later, because it may have to expand Ellipsis, etc. so it does not know at that point which dimension the index acts on).

@seberg seberg added 00 - Bug 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes component: numpy._core labels Aug 5, 2020
This test used to give a different broadcasting ValueError than the one from
other test.
@asmeurer
Copy link
Member Author

asmeurer commented Aug 5, 2020

I'm still a little shaky on the logic in that function, so you'll have to tell me if my updated comments are correct.

@asmeurer
Copy link
Member Author

asmeurer commented Aug 5, 2020

I added a changelog entry. I can't figure out how to build the docs (git submodule is doing something stupid), so I hope it renders OK.

Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>
@seberg
Copy link
Member

seberg commented Aug 6, 2020

@asmeurer no need, it seems fine, and most errors should be noted. You can actually press the ci/circleci: build artifact link and go to the page, which is: https://15017-908607-gh.circle-artifacts.com/0/doc/build/html/release/1.20.0-notes.html?highlight=release%20notes#boolean-array-indices-with-mismatching-shapes-now-properly-give-indexerror

Looks good to me. A comment for the tests might be nice, they are hard to understand why the different indx1, etc. are all wrong in slightly different ways. The last one is broadcastable but shape mismatch?

For anyone interested: I am planning to put this in. It does change a corner case of an all False boolean index of matching size but not shape into an error. It also changes some ValueError → IndexError, but I think we have been pretty liberal about such changes in the past.

@seberg seberg self-requested a review August 6, 2020 15:45
@asmeurer
Copy link
Member Author

asmeurer commented Aug 6, 2020

The last one is broadcastable but shape mismatch?

I guess that's what the difference is. I just found that it gives a different error, ValueError: non-broadcastable operand with shape (1,1,2) doesn't match the broadcast shape (1,2,2), so it presumably goes through a different code path.

@seberg seberg changed the title Remove incorrect logic from boolean indexing fast path BUG: Remove incorrect logic from boolean indexing fast path Aug 6, 2020
@seberg seberg changed the title BUG: Remove incorrect logic from boolean indexing fast path BUG: Raise correct errors in boolean indexing fast path Aug 6, 2020
@seberg
Copy link
Member

seberg commented Aug 6, 2020

Thanks @asmeurer, also for adding the final comments to the tests!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
00 - Bug 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes component: numpy._core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"ValueError: operands could not be broadcast together with shapes" error on invalid boolean array index
3 participants