DEP: Deprecate setting the strides attribute of a numpy array #28925

eendebakpt · May 8, 2025

Addresses part of #28800.

Changes are factored out of #28901.

jorenham · May 8, 2025

I'm having a bit of a déjà vu here 🤔

seberg

Thanks, the deprecation needs to be moved, and it would be nice if you can fix the habit of putting spaces around kwarg=value.

Otherwise, this mainly needs a release note now that explains the alternatives.

seberg · May 9, 2025

numpy/_core/src/multiarray/getset.c

@@ -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 ) {


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.

And here is a long line, which needs space fixes also.

If you are at it, adding a /* Deprecated NumPy 2.3, 2025-05-11 */ above it would also be nice.

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.

seberg · May 9, 2025

numpy/_core/tests/test_multiarray.py

@@ -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


The ones we shouldn't just delete when finalizing would be nice to just replace immediately, but also not a big thing.

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.

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.

Could you just change this? I.e. by moving the unwieldy or not, as_strided or np.ndarray call into the c= kwarg.
(Needs a bit of line breaking, I suppose black style will do)

seberg · May 9, 2025

numpy/_core/tests/test_multiarray.py

        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")


N.B./nit: you should be fine with catch_warnings now. Python fixed the bugs that made sup necessary.

seberg · May 9, 2025

numpy/_core/tests/test_multiarray.py

+            sup.filter(DeprecationWarning, "Assigning the 'data' attribute")
+            for s in attr:
+                with pytest.warns(DeprecationWarning):
+                    assert_raises(AttributeError, delattr, a, s)


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.)

seberg · May 9, 2025

And I forgot to say, thanks splitting it out is much easier considering how unwieldy especially the .dtype one is!

eendebakpt · May 9, 2025

Thanks, the deprecation needs to be moved, and it would be nice if you can fix the habit of putting spaces around kwarg=value.

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.

seberg · May 9, 2025

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.

eendebakpt · May 9, 2025

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 ruff check --select E501 | grep "Line too long" you can see that most long lines are in the 88 to 120 range.

seberg · May 9, 2025

PR created. The long lines is about 1500 changes which is maybe a bit too much? With ruff check --select E501 | grep "Line too long" you can see that most long lines are in the 88 to 120 range.

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!)

eendebakpt · May 9, 2025

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 ruff format, but that results in even more changes.

doc/release/upcoming_changes/28925.deprecation.rst

charris · May 10, 2025

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 numpy/random, then numpy/polynomial, and so on and check them off a list.

charris · May 10, 2025

BTW, please use hard line breaks when writing notes, don't depend on line wrap.

seberg

LGTM, a few nitpicks and things that would be nice to refactor now. But basically ready to go in, IMO, thanks!

seberg · May 10, 2025

numpy/_core/tests/test_multiarray.py

@@ -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


Could you just change this? I.e. by moving the unwieldy or not, as_strided or np.ndarray call into the c= kwarg.
(Needs a bit of line breaking, I suppose black style will do)

doc/release/upcoming_changes/28925.deprecation.rst

seberg · May 11, 2025

numpy/_core/src/multiarray/getset.c

@@ -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 ) {


If you are at it, adding a /* Deprecated NumPy 2.3, 2025-05-11 */ above it would also be nice.

seberg · May 11, 2025

numpy/_core/tests/test_regression.py

        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)


These ones would be nice to change, since the test is unrelated to the strides setting itself.

doc/release/upcoming_changes/28925.deprecation.rst

ngoldbaum

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.

ngoldbaum · May 12, 2025

doc/release/upcoming_changes/28925.deprecation.rst

+* `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.


I'm not sure what you mean by "manually setting strides" here. You mean passing strides to the ndarray constructor or something else? Please clarify.

Updated the text: we mean setting the strides on a numpy array directly.

ngoldbaum · May 12, 2025

numpy/_core/src/multiarray/getset.c

@@ -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 ) {


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.

jorenham · May 14, 2025

needs a rebase

Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>

numpy/_core/src/multiarray/getset.c

eendebakpt · May 15, 2025

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.

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 as_strided which is a good sign):

https://github.com/search?q=repo%3Ascipy%2Fscipy+strides+language%3APython&type=code&l=Python
https://github.com/search?q=repo%3Apandas-dev%2Fpandas%20strides&type=code

github-actions bot added the 07 - Deprecation label May 8, 2025

seberg reviewed May 9, 2025

View reviewed changes

seberg added the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label May 9, 2025

eendebakpt commented May 9, 2025

View reviewed changes

doc/release/upcoming_changes/28925.deprecation.rst Show resolved Hide resolved

seberg removed the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label May 10, 2025

seberg approved these changes May 11, 2025

View reviewed changes

eendebakpt commented May 11, 2025

View reviewed changes

doc/release/upcoming_changes/28925.deprecation.rst Show resolved Hide resolved

ngoldbaum reviewed May 12, 2025

View reviewed changes

eendebakpt and others added 9 commits May 15, 2025 19:34

DEP: Deprecate setting the strides attribute of a numpy array

18c02c8

update

687cf72

add missing changes

b32319e

ci

0fda654

review comments part 1

d7cfed3

Update doc/release/upcoming_changes/28925.deprecation.rst

730c9a9

Update doc/release/upcoming_changes/28925.deprecation.rst

5874636

Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>

review comments

2dad334

apply ruff

288690d

eendebakpt force-pushed the array_stride_set branch from 44b4a93 to 288690d Compare May 15, 2025 17:44

This comment has been minimized.

Sign in to view

fix merge issues

8502a2f

eendebakpt added 3 commits May 15, 2025 20:00

clarify release notes

0c4929e

update warning

61eee97

linter

141bbb7

eendebakpt force-pushed the array_stride_set branch from c64e8d1 to 141bbb7 Compare May 15, 2025 18:28

eendebakpt added 3 commits May 15, 2025 20:49

fix merge conflicts again

88950f2

merge conflicts

347b8d3

correct ruff version

4bbbfbc

charris reviewed May 15, 2025

View reviewed changes

numpy/_core/src/multiarray/getset.c Outdated Show resolved Hide resolved

review comment

60df1f7

eendebakpt added 2 commits May 18, 2025 22:11

Merge branch 'main' into array_stride_set

36a8ca0

ci

bce4d22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEP: Deprecate setting the strides attribute of a numpy array #28925

DEP: Deprecate setting the strides attribute of a numpy array #28925

eendebakpt commented May 8, 2025

jorenham commented May 8, 2025

seberg left a comment

seberg May 9, 2025

charris May 10, 2025 •

edited

Loading

seberg May 11, 2025

ngoldbaum May 12, 2025

seberg May 9, 2025

eendebakpt May 9, 2025

seberg May 9, 2025

seberg May 10, 2025

seberg May 9, 2025

seberg May 9, 2025

seberg commented May 9, 2025

eendebakpt commented May 9, 2025

seberg commented May 9, 2025

eendebakpt commented May 9, 2025

seberg commented May 9, 2025

eendebakpt commented May 9, 2025

charris commented May 10, 2025

charris commented May 10, 2025

seberg left a comment

seberg May 10, 2025

seberg May 11, 2025

seberg May 11, 2025

ngoldbaum left a comment

ngoldbaum May 12, 2025

eendebakpt May 16, 2025

ngoldbaum May 12, 2025

jorenham commented May 14, 2025

This comment has been minimized.

eendebakpt commented May 15, 2025

Search code, repositories, users, issues, pull requests...

DEP: Deprecate setting the strides attribute of a numpy array #28925

Are you sure you want to change the base?

DEP: Deprecate setting the strides attribute of a numpy array #28925

Conversation

eendebakpt commented May 8, 2025

jorenham commented May 8, 2025

seberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charris May 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seberg commented May 9, 2025

eendebakpt commented May 9, 2025

seberg commented May 9, 2025

eendebakpt commented May 9, 2025

seberg commented May 9, 2025

eendebakpt commented May 9, 2025

charris commented May 10, 2025

charris commented May 10, 2025

seberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngoldbaum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorenham commented May 14, 2025

This comment has been minimized.

eendebakpt commented May 15, 2025

charris May 10, 2025 •

edited

Loading