WIP: Actually try deprecating non-tuple nd-indices #4434
Conversation
numpy/core/src/multiarray/mapping.c
8000
Outdated
| @@ -228,6 +228,10 @@ | |||
|
|
|||
| if (make_tuple) { | |||
| /* We want to interpret it as a tuple, so make it one */ | |||
| if (DEPRECATE("interpreting a sequence (not baseclass tuple) as a " | |||
| "multidimensional indexing is deprecated.") < 0) { | |||
There was a problem hiding this comment.
How about: "using a non-tuple sequence for multidimensional indexing is deprecated; use arr[tuple(foo)] instead of arr[foo]"
There was a problem hiding this comment.
Sure. Will also add a test. Not sure about the failure, I think gradient possibly doesn't work with multidimensional datetime input, but I find it a bit puzzling that advanced indexing worked and normal doesn't. It might have something to do with arr[[0]] returning an array while arr[0,] returns a scalar...
|
I like it, and no-one objected on the list. Not sure what's up with the test failure, but otherwise LGTM... |
|
Though, it would be good to also have:
|
|
This PR would change the behavior of this code? |
|
No, both of those are unaffected. All it does changes is to start issuing a On Tue, Mar 4, 2014 at 7:17 PM, argriffing notifications@github.com wrote:
Nathaniel J. Smith |
|
Updated (made it a futurewarning, too). But the datetime gradient issue is open, so... |
|
This should work now (though it is not unlikely that there is a |
|
Note, this also changes the behaviour of tuple subclasses (or warns about a change). Tuple subclasses apparently were always handled like tuples before. I don't care much either way but the new test will need fixing (tests would fail if rerun currently), and before we put this in, we need to decide on it. |
|
☔ The latest upstream changes (presumably #5990) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I have updated this (resolved merge conflict. Obviously it is not ready, since the 1.9. release notes are probably not the right place ;), but could discuss this again. |
|
☔ The latest upstream changes (presumably #6047) made this pull request unmergeable. Please resolve the merge conflicts. |
Such indexing has problem with ambiguouity for example whether `arr[[[0], [1]]]` is `arr[[0], [1]]` or `arr[asarray([[0], [1]])]` Adds the tuple cast where it is necessary (at the time of writing this, it is not unlikely that there are some hidden ones missing).
|
Updated this a bit, but getting some weirder errors, weeding out everything, especially in the masked array stuff seems a bit tricky. Anyone who wants to pick it up, be my guest, it is getting a bit annoying :). |
| @@ -39,6 +39,13 @@ DeprecationWarning to error | |||
| FutureWarning to changed behavior | |||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | |||
|
|
|||
| * Multidimensional indexing with anything but a base class tuple is | |||
| deprecated. This means that code such as ``arr[[slice(None)]]`` has to | |||
| be changed to ``arr[tuple([slice(None)])]``. This is necessary to avoid | |||
There was a problem hiding this comment.
I would write: arr[(slice(None),)] or simply arr[slice(None),]
There was a problem hiding this comment.
Hehe, yeah, though that would somewhat defeat the purpose ;). should make the example longer... Anyway, doesn't look like 1.11 change.
Hmmm, reading my last comment, part of me wonders why the masked arrays are always so tricky when doing indexing changed.
EDIT: should make the list explicit.
|
☔ The latest upstream changes (presumably #7134) made this pull request unmergeable. Please resolve the merge conflicts. |
Recommended type for nd-indexing is a tuple. See numpy#4434
Recommended type for nd-indexing is a tuple. See numpy#4434
| } | ||
| else if (make_tuple == 1) { | ||
| if (DEPRECATE_FUTUREWARNING( | ||
| "multi dimensional indexing tuple was not base class. " |
There was a problem hiding this comment.
Is there a good reason to deprecate this? Allowing subclasses of tuple doesn't seem unreasonable, as there's still no ambiguity
There was a problem hiding this comment.
No, probably not, I probably wasn't sure and just thought I will go with the extreme version and make it less extreme from there ;).
|
@seberg: Can you make this editable so that I can pick it up? |
| # just set y equal to the the array `f`. | ||
| if f.dtype.char in ["M", "m"]: | ||
| y = f.view('int64') | ||
| y = f.view(otype) |
There was a problem hiding this comment.
there is some weird problems in gradient that made it work only if it did the wrong, fancy, indexing. I guess this maybe somehow fixed/worked around it, it is possible that the commit message explains more, but likely not.
There was a problem hiding this comment.
Would not rule out that other datetime fixes in the meantime may have made this better, or that changing things at another place would be more correct.
|
Anyway, the problem with this is less the work to make it work, but more making the decision to do this very noisy deprecation which affects quite a lot of things. |
|
One thing we could do right now is merge all the fixes that make numpy only ever index with tuples internally. That way subclassers don't need to deal with this "feature" if all they care about is numpy itself |
|
Yeah, I guess so, though unless you add a compile time option to give the warning or so (and use it during testing), there will be no guarantee for it in the future and its useless. There is a bit of a downer, that manual tuple conversion is slower (probably because python has kwarg parsing overhead or so), but there is no point being annoyed by it I suppose. |
175e0f1 to
81aa766
Compare
|
Should we close #4434? |
|
Sure, why not, could always reopen anyway if there were a reason for it. |
Just here for discussion. I am not sure it isn't too brutal. Also there is a test failure right now, which I think is probably another bug...