DEP: Ensure indexing errors will be raised even on empty results#15900
DEP: Ensure indexing errors will be raised even on empty results#15900mattip merged 1 commit intonumpy:masterfrom
Conversation
342f4d4 to
a69b6ea
Compare
3ca3d64 to
b0b7fd0
Compare
|
Unless we do not want to skip the deprecation, I guess is not quite trivial, but I think it should be ready. |
numpy/core/src/multiarray/mapping.c
Outdated
There was a problem hiding this comment.
| "Invalid index into empty indexing result found. " | |
| "Invalid index into empty array found. " |
The indexing is into the input, not the result, isn't it?
eric-wieser
left a comment
There was a problem hiding this comment.
Change looks fine, wording of the release note and error message could maybe be improved, currently it confuses me a little.
b0b7fd0 to
cba6f64
Compare
|
Yeah, tricky, tried to rephrase both. |
cba6f64 to
79bbe76
Compare
|
Just noticed this, tests should run through now (forgot to modify the message in the test), maybe you can have a second look @eric-wieser . |
5e7386f to
bdc6f69
Compare
|
Would you mind adding a test that the case we were discussing before is now not deprecated? I think the |
|
@eric-wieser I changed the old test from testing that its deprecated to testing that its not deprecated. |
mattip
left a comment
There was a problem hiding this comment.
Small tweaks to the release note, otherwise LGTM.
e1c1b33 to
07f17a7
Compare
numpy/core/src/multiarray/mapping.c
Outdated
There was a problem hiding this comment.
Use
warnings.simplefilter('error')to raise the warning
Which warning does "the warning" refer to, this DeprecationWarning or some other?
There was a problem hiding this comment.
expanded the "the" to "this DeprecationWarning" and squashed the commits.
There was a problem hiding this comment.
So now I am confused. This message is the DeprecationWarning. It seems redundant, or am I missing something?
Also, your rebase caught a change in a submodule ...
There was a problem hiding this comment.
Ahh, maybe you meant "to turn this warning into an error and get more details ..."
There was a problem hiding this comment.
ah, dang, yes and thanks for noticing the submodule change, commit -a is dangerous right now :)
There was a problem hiding this comment.
Thanks, yes, adopted... I could just delete the whole thing as well.
1ee64f8 to
b095232
Compare
numpy/core/src/multiarray/mapping.c
Outdated
Previously, when the indexing result was empty, no check was done for backward compatibility with pre 1.9 (assumingly). This may have been only necessary when the outer iteration is empty as opposed to when just the inner iteration is empty, though. In any case, it is arguably buggy to ignore indexing errors in this case. Since there may have been a reason back in the day, and this is probably extremely rare, optiming for a brief deprecation period for now. Closes numpygh-15898 Co-authored-by: Eric Wieser <wieser.eric@gmail.com> Co-authored-by: Matti Picus <matti.picus@gmail.com>
b095232 to
381a2f5
Compare
|
Thanks @seberg |
Previously, when the indexing result was empty, no check was done
for backward compatibility with pre 1.9 (assumingly).
This may have been only necessary when the outer iteration is empty
as opposed to when just the inner iteration is empty, though.
In any case, it is arguably buggy to ignore indexing errors in this
case. Since there may have been a reason back in the day, and this
is probably extremely rare, optiming for a brief deprecation period
for now.
Closes gh-15898
This opts for a deprecation, but we could decide to skip the deprecation, I suppose we could also decide to make it a
VisibleDeprecationWarning. I am a bit surprised there are no test for this, since I obviously was aware of this behaviour when I wrote the indexing code... Although it is possible that it was part of those "Automated" tests, and then got revised away accidentally.