Repository navigation
MAINT: Remove __setslice__ and __getslice__ - #8592
Conversation
28040f0 to
1a7fe64
Compare
This code was only here for Python 2.5 compatibility, but numpy requires 2.7 at minimum
1a7fe64 to
994f8a1
Compare
charris
added
03 - Maintenance
component: documentation
component: numpy._core
component: numpy.lib
component: numpy.ma
| MaskedArray.__setitem__ | ||
| MaskedArray.__delitem__ | ||
| MaskedArray.__getslice__ | ||
| MaskedArray.__setslice__ |
There was a problem hiding this comment.
Note that getslice and setslice are also in the index. Grep for the terms.
There was a problem hiding this comment.
Right, but without the __. I was under the impression that was just a coincidental reference to something else. If that's not the case, I'll wipe that one too
|
I'm a bit concerned about backward compatibility, note the reference from @mhvk. Both Pandas and SciPy look good, however, so maybe not worth worrying about. |
|
Needs a mention in the 1.13.0 release notes in the compatibility section. |
|
I'm not convinced this would have any compatibility issues beyond |
|
I'll do some experiments to characterize exactly what the effect is |
|
@charris: Changelog added. Compatibility definitely only affects Python 2.7. Experiments so far:
|
| .. index:: | ||
| triple: ndarray; special methods; getslice | ||
| triple: ndarray; special methods; setslice | ||
| triple: ndarray; special methods; get slice |
There was a problem hiding this comment.
Should be an ndarray method. getitem and setitem are probably what should be here.
There was a problem hiding this comment.
Why not __getitem__ and __setitem__ ?
Replaces getslice and setslice which have been removed.
|
Made a small fix and will commit when tests finish. Yeah, I forgot |
|
@charris: didn't know you could edit the PR! Is that new? |
|
No, not new but not often used. You can disable it when you make the PR, there is a button somewhere... |
|
Thanks @eric-wieser . |
|
@pv Looks like that. Does that show up in Python3? If not, I think it indicates that getitem needs some improvement. |
|
Frankly I doubt there is much you can do (easily). Slicing is a pretty fast operation, so simply creating a few extra python objects and more complex getitem logic may already add a substantial slowdown. But maybe its worth a look.... |
|
Perhaps there's an argument for |
| Py_DECREF(ret); | ||
| return NULL; | ||
| } | ||
| PyArray_UpdateFlags(ret, NPY_ARRAY_UPDATE_ALL); |
There was a problem hiding this comment.
Why does this line exist here but not in get_view_from_index?
There was a problem hiding this comment.
Hmmm, no idea why this should be needed here. Maybe it was just being very careful, or a git blame might help to see when the line was added if it was. The array creation should set all the flags correctly already, except maybe writable, but that is correct.
|
Another bonus of killing this: #python 2.7
>>> x = np.array(0)
>>> x[-1:]
TypeError: len() of unsized object
>>> x[np.s_[-1:]]
IndexError: too many indices for array |
|
this change needs to be reverted. we do not break backward compatibility for cosmetic reasons! |
This reverts commit 994f8a1 and bec587c from numpygh-8592. This is essentially a cosmetic change that breaks backward compatibility with in use programs like astropy 1.3.
charris
changed the title
__setslice__ and __getslice__
This code was only here for Python 2.5 compatibility, but numpy requires 2.7 at minimum.
This involves:
__[gs]etslice__method from python filessq_sliceandsq_slice_assslots from C files, so that the python files do not have to override the base implementations