BUG: Raise correct errors in boolean indexing fast path#17010
Conversation
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.
|
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). |
This test used to give a different broadcasting ValueError than the one from other test.
|
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. |
|
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>
|
@asmeurer no need, it seems fine, and most errors should be noted. You can actually press the Looks good to me. A comment for the tests might be nice, they are hard to understand why the different 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. |
I guess that's what the difference is. I just found that it gives a different error, |
|
Thanks @asmeurer, also for adding the final comments to the tests! |
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?)