DEP: Deprecate setting the strides attribute of a numpy array #28925
Conversation
|
I'm having a bit of a déjà vu here 🤔 |
| @@ -116,6 +116,10 @@ array_strides_get(PyArrayObject *self, void *NPY_UNUSED(ignored)) | |||
| static int | |||
| array_strides_set(PyArrayObject *self, PyObject *obj, void *NPY_UNUSED(ignored)) | |||
| { | |||
| if( DEPRECATE("Setting the strides on a NumPy array has been deprecated in NumPy 2.3.") < 0 ) { | |||
There was a problem hiding this comment.
It may be nice to point out the work-arounds, but considering that stride setting is a bit niche maybe we can also just do that in the release note.
Something like: See np.lib.stride_tricks.strided_window_view and np.lib.stride_tricks.as_strided.
| @@ -8290,7 +8297,8 @@ def test_padded_struct_array(self): | |||
| def test_relaxed_strides(self, c=np.ones((1, 10, 10), dtype='i8')): | |||
| # Note: c defined as parameter so that it is persistent and leak | |||
| # checks will notice gh-16934 (buffer info cache leak). | |||
| c.strides = (-1, 80, 8) # strides need to be fixed at export | |||
| with pytest.warns(DeprecationWarning): | |||
| c.strides = (-1, 80, 8) # strides need to be fixed at export | |||
There was a problem hiding this comment.
The ones we shouldn't just delete when finalizing would be nice to just replace immediately, but also not a big thing.
There was a problem hiding this comment.
I did not change this one because I was not sure about the test. If we use the stride_tricks then we are creating a new array and might not catch the memory leak. Probably fine either way.
There was a problem hiding this comment.
Yes, the comment makes sense if a bit hard to follow. That just means you need to move the as_strided call into the argument, though.
numpy/_core/tests/test_multiarray.py
Outdated
| with suppress_warnings() as sup: | ||
| sup.filter(DeprecationWarning, "Assigning the 'data' attribute") | ||
| for s in attr: | ||
| assert_raises(AttributeError, delattr, a, s) | ||
|
|
||
| attr = ['strides'] | ||
| with suppress_warnings() as sup: | ||
| sup.filter(DeprecationWarning, "Assigning the 'data' attribute") |
There was a problem hiding this comment.
N.B./nit: you should be fine with catch_warnings now. Python fixed the bugs that made sup necessary.
numpy/_core/tests/test_multiarray.py
Outdated
| sup.filter(DeprecationWarning, "Assigning the 'data' attribute") | ||
| for s in attr: | ||
| with pytest.warns(DeprecationWarning): | ||
| assert_raises(AttributeError, delattr, a, s) |
There was a problem hiding this comment.
this is overly complicated (probably including that filter above), because you are giving the warning when it'll raise anyway.
You should just move the warning to after the error for deleting (value is NULL).
(Or actually, move that error to the very beginning, since we are not bound by C89 style "declarations first" anymore.)
|
And I forgot to say, thanks splitting it out is much easier considering how unwieldy especially the |
This is not flagged by the linter. It is E251 (https://docs.astral.sh/ruff/rules/unexpected-spaces-around-keyword-parameter-equals/) should I add that to the configuration in a different PR? Runnnig it on the current codebase gives 52 changes. |
Yeah, that would be great. I am very confused why this isn't enabled. IIRC, long lines also don't seem to get flagged. |
PR created. The long lines is about 1500 changes which is maybe a bit too much? With |
Yeah, I hadn't realized that we don't have an lint on the newly added lines anymore, that used to be the setup. So, I think we should probably also just do it, and just before branching 2.3 is the right time (simplifies backports), but I'll let @charris make the decision. (Or it would be nice to fix this and re-introduce a more strict check only on changed code, that should not be hard!) |
|
Hmm. Ruff cannot automatically fix long lines (E501). So this would mean manually changing 1500 lines which is not impossible, but certainly not a quick job. There is also |
Addresses part of #28800.
Changes are factored out of #28901.