DEP: Deprecate non-tuple nd-indices #9686
Conversation
17a20e5 to
175e0f1
Compare
There was a problem hiding this comment.
Most comments just attempt at making a tuple to start with. The many occurrences in our own code do make me wonder slightly whether this is in fact a good idea. I fear the construction of lists consisting of lots of slice(None) plus a real slice is going to be common also outside of numpy...
numpy/fft/fftpack.py
Outdated
| @@ -69,13 +69,13 @@ def _raw_fft(a, n=None, axis=-1, init_function=fftpack.cffti, | |||
| if s[axis] > n: | |||
| index = [slice(None)]*len(s) | |||
There was a problem hiding this comment.
Maybe nicer to just fix the creation of the index:
index = (slice(None),) * axis + (slice(0, n),)
then, no need to use tuple b
8000
elow.
doc/release/1.14.0-notes.rst
Outdated
| @@ -21,6 +21,12 @@ New functions | |||
| Deprecations | |||
| ============ | |||
|
|
|||
| * Multidimensional indexing with anything but a base class tuple is | |||
| deprecated. This means that code such as ``arr[[slice(None)]]`` has to | |||
There was a problem hiding this comment.
How about giving an example with a slightly more useful case, e.g., [slice(None), slice(0, 10)] -> tuple(slice(None), slice(0, 10)) (or even omit the tuple in that case).
numpy/core/tests/test_memmap.py
Outdated
| @@ -126,7 +126,7 @@ def test_arithmetic_drops_references(self): | |||
| def test_indexing_drops_references(self): | |||
| fp = memmap(self.tmpfp, dtype=self.dtype, mode='w+', | |||
| shape=self.shape) | |||
| tmp = fp[[(1, 2), (2, 3)]] | |||
| tmp = fp[((1, 2), (2, 3))] | |||
There was a problem hiding this comment.
Here, the parenthesis are not necessary, correct?
numpy/fft/fftpack.py
Outdated
| @@ -69,13 +69,13 @@ def _raw_fft(a, n=None, axis=-1, init_function=fftpack.cffti, | |||
| if s[axis] > n: | |||
| index = [slice(None)]*len(s) | |||
| index[axis] = slice(0, n) | |||
| a = a[index] | |||
| a = a[tuple(index)] | |||
| else: | |||
| index = [slice(None)]*len(s) | |||
| index[axis] = slice(0, s[axis]) | |||
There was a problem hiding this comment.
And same here.
index = (slice(None),) * axis + (slice(0, s[axis]),)
numpy/lib/function_base.py
Outdated
| @@ -1745,7 +1745,7 @@ def gradient(f, *varargs, **kwargs): | |||
| slice4[axis] = slice(2, None) | |||
There was a problem hiding this comment.
If up to me, I'd just create a base prefix (slice(None),) * axis here and add the 1,2,3,4 as need below - which I think would make for clearer code. But that is getting well beyond the immediate purpose of this PR, so feel free to ignore.
numpy/ma/core.py
Outdated
| @@ -5483,6 +5483,7 @@ def sort(self, axis=-1, kind='quicksort', order=None, | |||
| else: | |||
| idx = list(np.ix_(*[np.arange(x) for x in self.shape])) | |||
| idx[axis] = sidx | |||
| idx = tuple(idx) | |||
There was a problem hiding this comment.
Maybe
idx = np.ix_(*[(sidx if i == axis else np.arange(self.shape[i])) for i in range(self.ndim)])
well, maybe not really better at all.
numpy/ma/extras.py
Outdated
| @@ -723,6 +723,7 @@ def _median(a, axis=None, out=None, overwrite_input=False): | |||
| # as median (which is mean of empty slice = nan) | |||
| indexer = [slice(None)] * asorted.ndim | |||
| indexer[axis] = slice(0, 0) | |||
| indexer = tuple(indexer) | |||
There was a problem hiding this comment.
indexer = (slice(None),) * axis + (slice(0, 0),)
numpy/ma/extras.py
Outdated
| @@ -784,6 +785,7 @@ def replace_masked(s): | |||
| if np.issubdtype(asorted.dtype, np.inexact): | |||
| # avoid inf / x = masked | |||
| s = np.ma.sum([low, high], axis=0, out=out) | |||
| print(repr(s.data)) | |||
There was a problem hiding this comment.
Nice catch! Must have been in my working copy or stash or something
numpy/ma/extras.py
Outdated
| @@ -1657,7 +1659,7 @@ def flatnotmasked_contiguous(a): | |||
| if not k: | |||
| result.append(slice(i, i + n)) | |||
| i += n | |||
| return result or None | |||
| return tuple(result) or None | |||
There was a problem hiding this comment.
Also update the docstring! (It states a list is returned)
numpy/ma/extras.py
Outdated
| result.append(flatnotmasked_contiguous(a[idx]) or None) | ||
| return result | ||
| result.append(flatnotmasked_contiguous(a[tuple(idx)]) or None) | ||
| return tuple(result) |
|
Just some notes from a performance perspective: 67E6
>>> a = np.zeros((2,)*10)
>>> sln = (slice(None),) # cache this to try and maximize speed
>>> %timeit a[sln*3 + (1,) + sln*6]
933 ns ± 20 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
>>> sl = list(sln * 10)
>>> %timeit sl[3] = 1; a[tuple(sl)]
858 ns ± 14.7 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
>>> %timeit sl[3] = 1; a[sl] # passing a list is still fastest though
748 ns ± 27.3 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each) |
|
Fun, it used to be quite a bit slower, I guess python optimized the
keyword parsing in `tuple` or something.
|
|
Weirdly, writing it out explicitly is slowest of all: |
|
Dunno, might be unfair since python has to create the slice objects inside the timeit, etc. |
|
The performance aspect probably doesn't matter so much; my comments were more about trying to make the code cleaner, with less conversions. But I must admit that I've wondered why direct list comprehension is so much faster than doing it inside a function: |
|
Sounds like this might need more discussion. |
|
Pushing off to 1.15. |
175e0f1 to
a4c0c57
Compare
|
@mhvk: I'd prefer to leave the different tuple generation strategy to a different PR, and keep this change as small as possible. |
397c822 to
8f469b3
Compare
numpy/core/src/multiarray/mapping.c
Outdated
| "interpreted as an array index, `arr[np.array(seq)]`, " | ||
| "which will result either in an error or a different " | ||
| "result.") < 0) { | ||
| return -1; |
There was a problem hiding this comment.
Think I might have missed some DECREFs here.
|
Base switch above to recompute the diff. Shame there's no button to do that. |
8f469b3 to
baac902
Compare
|
This maybe needs a doc change to https://docs.scipy.org/doc/numpy/reference/arrays.indexing.html#advanced-indexing |
|
@eric-wieser Want to make that change or can I put this in. |
|
Is there a plan for finishing this deprecation cycle? Would 1.17 be too soon to change this? |
|
My gut feel is to leave this till (at least) 1.18 - this was quite an intrusive deprecation, and the PR references above suggest that downstream packages are still running into it every few months. Perhaps the deprecation NEP has a clear stance here. Is there some reason you're hoping to get rid of this sooner? My main goal with this PR was to stop users writing code in this style, so I could stop trying to replicate it in my own |
|
Right now, But I agree, it seems that we should wait a bit longer before changing this. |
|
Do the |
|
They implement |
|
I think the presence of |
It won't have an opinion on any specific cases. That was a clear message from review: just express the principles not current cases. I'm planning to get back to that NEP soon by the way. I agree with waiting till 1.18 |
|
We might be able to finish the deprecation now for everything except We could also consider bumping this to an |
I like that idea, if we can do that for 1.17 that will probably flush out many more issues for end users. |
This behavior has been deprecated since NumPy 1.15 (numpy#9686).
This behavior has been deprecated since NumPy 1.15 (numpy#9686).
This behavior has been deprecated since NumPy 1.15 (numpy#9686).
This behavior has been deprecated since NumPy 1.15 (numpy#9686).
This behavior has been deprecated since NumPy 1.15 (numpy#9686).
This behavior has been deprecated since NumPy 1.15 (numpy#9686).
This behavior has been deprecated since NumPy 1.15 (numpy#9686).
This behavior has been deprecated since NumPy 1.15 (numpy#9686).
This behavior has been deprecated since NumPy 1.15 (numpy#9686).
This behavior has been deprecated since NumPy 1.15 (numpy#9686).
A reminder of the purpose of this - currently we allow both
arr[[None, 0]]andarr[(None, 0)]to mean the same thing, yetarr[[0, 0]]andarr[(0, 0)]mean different things. This makes it super hard to make a compliant subclass, or duck array.By deprecating this feature, we force downstream library code to stop using it, which in turn makes that library code more compatible with subclasses and duck types.
This reapplies @seberg's #4434, with the following changes:
tuplesubclasses are not deprecated, because supportingnamedtuples seems possibly usefulFutureWarnings, since determining hownp.array(...)will treat a sequence is expensive - it's easier to just say "this will error ifnp.array(seq)would"As of #10618, none of our code relies on this "feature"