Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork10.9k
DEP: Deprecate setting the strides attribute of a numpy array#28925
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
I'm having a bit of a déjà vu here 🤔 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks, the deprecation needs to be moved, and it would be nice if you can fix the habit of putting spaces aroundkwarg=value.
Otherwise, this mainly needs a release note now that explains the alternatives.
numpy/_core/src/multiarray/getset.c Outdated
| @@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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: Seenp.lib.stride_tricks.strided_window_view andnp.lib.stride_tricks.as_strided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
And here is a long line, which needs space fixes also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If you are at it, adding a/* Deprecated NumPy 2.3, 2025-05-11 */ above it would also be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree the warning needs a suggestion for what to do instead and maybe a pointer to stride_tricks if someone really does need to mutate the strides of an existing array.
numpy/_core/tests/test_multiarray.py Outdated
| @@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I did not change this one because I was not sure about the test. If we use thestride_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, the comment makes sense if a bit hard to follow. That just means you need to move theas_strided call into the argument, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could you just change this? I.e. by moving the unwieldy or not,as_strided ornp.ndarray call into thec= kwarg.
(Needs a bit of line breaking, I suppose black style will do)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
N.B./nit: you should be fine withcatch_warnings now. Python fixed the bugs that madesup 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 toafter the error for deleting (value isNULL).
(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 thenewly 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 |
Uh oh!
There was an error while loading.Please reload this page.
For fixing long line, I'd probably start with non-test files and do it in bits. One of the problems is data in tests, which we like to keep aligned for readability. But yeah, there are a lot of long lines. Some might be easy to fix, but others might be a pain, which why I suggest doing it in parts. For example, do |
BTW, please use hard line breaks when writing notes, don't depend on line wrap. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM, a few nitpicks and things that would be nice to refactor now. But basically ready to go in, IMO, thanks!
numpy/_core/tests/test_multiarray.py Outdated
| @@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could you just change this? I.e. by moving the unwieldy or not,as_strided ornp.ndarray call into thec= kwarg.
(Needs a bit of line breaking, I suppose black style will do)
Uh oh!
There was an error while loading.Please reload this page.
numpy/_core/src/multiarray/getset.c Outdated
| @@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If you are at it, adding a/* Deprecated NumPy 2.3, 2025-05-11 */ above it would also be nice.
numpy/_core/tests/test_regression.py Outdated
| assert np.lexsort((xs,), axis=0).shape[0] == 0 | ||
| xs.shape = (2, 0) | ||
| xs.strides = (16, 16) | ||
| with pytest.warns(DeprecationWarning): | ||
| xs.strides = (16, 16) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
These ones would be nice to change, since the test is unrelated to the strides setting itself.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Have you tried checking whether this impacts downstream projects? It might be polite to check e.g. the scipy, matplotlib, pandas, and scikit-learn to see how much noise this causes in their tests.
| * `np.lib.stride_tricks.as_strided` for the general case, | ||
| * or the `np.ndarray` constructor (``buffer`` is the original array) for a light-weight version. | ||
| Manually setting strides can be useful but needs to be done with care as it can lead to unsafe memory use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure what you mean by "manually setting strides" here. You mean passingstrides to the ndarray constructor or something else? Please clarify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Updated the text: we mean setting the strides on a numpy array directly.
numpy/_core/src/multiarray/getset.c Outdated
| @@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree the warning needs a suggestion for what to do instead and maybe a pointer to stride_tricks if someone really does need to mutate the strides of an existing array.
needs a rebase |
Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>
This comment has been minimized.
This comment has been minimized.
Uh oh!
There was an error while loading.Please reload this page.
scikit-learn tests pass locally, matplotlib has one failing test (lib/matplotlib/tests/test_backend_tk.py::test_canvas_focus) which seems unrelated to this PR. Searching for strides in the github repos of scipy and pandas gives no obvious places where the strides attribute is set (but I do see usage of https://github.com/search?q=repo%3Ascipy%2Fscipy+strides+language%3APython&type=code&l=Python |
Addresses part of#28800.
Changes are factored out of#28901.