BUG: fix handling of copy keyword argument when calling array #25922

ngoldbaum · 2024-03-03T23:24:55Z

This applies @rgommers draft changes from the issue description and adds a couple more fixes on top.

Replaces internal usages of never_copy with copy that can be -1 (corresponding to None), 0 (False), and 1 (True). This allows the array coercion machinery and PyArray_FromArrayAttr_int to support passing copy=True (although right now that will never happen).
Only passes a copy keyword if copy is not the default value.
Fixes a bug in checking for the error message
Removes an incorrect usage of Py_SetRef (sorry about suggesting that in the first place 😬 )
Fixes some reference counting issues
Tests that the old deprecated calling convention still works.

ngoldbaum · 2024-03-03T23:41:54Z

@oscarbenjamin it looks like there are still some sympy test failures with this PR applied, but I think they'll be fixed once scipy is updated (the failures have tracebacks that end in scipy).

FAILED sympy/physics/control/tests/test_control_plots.py::test_pole_zero - assert False
FAILED sympy/physics/quantum/tests/test_identitysearch.py::test_is_scalar_sparse_matrix - ValueError: Unable to avoid copy while creating an array from given array.
FAILED sympy/physics/quantum/tests/test_identitysearch.py::test_is_reducible - ValueError: Unable to avoid copy while creating an array from given array.
FAILED sympy/physics/quantum/tests/test_identitysearch.py::test_bfs_identity_search - ValueError: Unable to avoid copy while creating an array from given array.
FAILED sympy/physics/quantum/tests/test_identitysearch.py::test_bfs_identity_search_xfail - ValueError: Unable to avoid copy while creating an array from given array.
FAILED sympy/solvers/tests/test_simplex.py::test_lp - ImportError: cannot import name '_np' from 'sympy.solvers.inequalities' (/Users/goldbaum/D...
FAILED sympy/utilities/tests/test_lambdify.py::test_scipy_sparse_matrix - ValueError: Unable to avoid copy while creating an array.

(although not sure what's up with test_control_plots and test_simplex).

I tried testing scipy with this PR applied, but it looks like scipy isn't buildable at the moment because of the descr->f removal.

oscarbenjamin · 2024-03-04T00:14:40Z

Thanks @ngoldbaum. If changes are needed on SymPy's end then that's fine and I can make them. For now though perhaps the test failures in sympy are some measure of potential breakage that could be caused for other code.

numpy/_core/src/multiarray/ctors.c

charris · 2024-03-04T02:00:50Z

Not clear what is going on with the Linux SIMD tests / without optimizations, I think copy should be recognized as a keyword and it didn't fail before. Maybe the problem is with something else called with the keyword.

ngoldbaum · 2024-03-04T02:07:26Z

I can't reproduce that failure locally, but I see that it's running on python 3.12-dev, which seems odd now that python 3.12 is out. Let's see what happens if I use the release python version.

ngoldbaum · 2024-03-04T02:23:05Z

Hmm, nope. I just tried running python 3.12.1 and I can reproduce this there though. I guess an API changed...

ngoldbaum · 2024-03-04T02:46:07Z

It looks like on Python 3.12, the exception object you get back from PyErr_Fetch doesn't pass PyUnicode_Check, so I updated the implementation to explicitly convert the exception object into a string, which seems to work on 3.11 and 3.12. Hopefully that makes all the CI happy.

ngoldbaum · 2024-03-04T02:47:44Z

(we should probably add more CI coverage on python 3.12 if that's the only runner on that version)

charris · 2024-03-04T03:52:44Z

Thanks Nathan, good sleuthing.

I guess an API changed

Nasty. But I guess we can understand :)

rgommers · 2024-03-04T07:53:23Z

Thanks a lot for the quick fix Nathan!

mtsokol · 2024-03-04T09:21:37Z

numpy/_core/src/multiarray/ctors.c

            return NULL;
        }
    }

+    Py_DECREF(kwargs);


Should we have Py_DECREF(args); next to kwargs? Similarly to:

numpy/numpy/_core/src/multiarray/arrayfunction_override.c

Line 240 in 9d69a62

Py_DECREF(args);

And this is the only comment that I have.

jorisvandenbossche · 2024-03-05T12:40:16Z

doc/release/upcoming_changes/25168.change.rst

+NumPy will pass ``copy`` to the ``__array__`` special method in situations where
+it would be set to a non-default value (e.g. in a call to
+``np.asarray(some_object, copy=False)``). Currently, if an
+unexpected keyword argument error is raised after this, NumPy will print a
+warning and re-try without the ``copy`` keyword argument. Implementations of
+objects implementing the ``__array__`` protocol should accept a ``copy`` keyword
+argument with the same meaning as when passed to `numpy.array` or
+`numpy.asarray`.


Happy to move it to a new issue, but thinking through how to update some __array__ implementations, I am wondering: what are the expectations from numpy how an __array__ implementations handles this keyword fully or partially?

When copy=True is being passed, does numpy assume that the __array__ implementation already made that copy? But what if the implementation defers dtype casting to numpy (which is currently possible), then it might be a waste to already copy up front?

Great question. Worth a new issue indeed I think.

But what if the implementation defers dtype casting to numpy (which is currently possible)

Deprecated though, right? The signature should already have included the dtype keyword, even though it was still working to not have that.

then it might be a waste to already copy up front?

What we want to achieve I think is that only a single copy is ever made. We can have two now in many cases, because the other library may copy from its internal implementation into an array, and then numpy will make another copy. The only way to get it right is for the other library to do the copy if copy=True, and for numpy not to do so if the method call including copy=True succeeded.

Created #25941, in case you want to move over your comments as well

BUG: fix handling of copy keyword argument when calling __array__

acf49fe

ngoldbaum requested a review from mtsokol March 3, 2024 23:24

github-actions bot added the 00 - Bug label Mar 3, 2024

ngoldbaum added this to the 2.0.0 release milestone Mar 3, 2024

charris reviewed Mar 4, 2024

View reviewed changes

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

MNT: use an interned string in __array__ retry error checking

1ab56b8

MNT: refactor to support python 3.12

b847453

ngoldbaum force-pushed the array-copy-fix branch from b86aabf to b847453 Compare March 4, 2024 02:44

charris merged commit 95684b6 into numpy:main Mar 4, 2024

mattip mentioned this pull request Mar 4, 2024

Coordination: last items before branching 2.0.x #25918

Closed

10 tasks

neutrinoceros mentioned this pull request Mar 4, 2024

numpy 2.0: a possible API inconsistency in numpy.ndarray.astype (copy semantics) ? #25925

Closed

mtsokol reviewed Mar 4, 2024

View reviewed changes

ngoldbaum mentioned this pull request Mar 4, 2024

BUG: fix reference leak in PyArray_FromArrayAttr_int #25931

Merged

This was referenced Mar 4, 2024

[TST] Upcoming dependency test failures matplotlib/matplotlib#27844

Closed

Add dtype/copy args to internal testing class matplotlib/matplotlib#27862

Merged

jorisvandenbossche reviewed Mar 5, 2024

View reviewed changes

jorisvandenbossche mentioned this pull request Mar 5, 2024

Expected behaviour regarding new copy keyword in __array__ implementations #25941

Closed

jakevdp mentioned this pull request Mar 14, 2024

BUG: NumPy 2.0.0b1 leaks reference counts in np.array #26020

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: fix handling of copy keyword argument when calling array #25922

BUG: fix handling of copy keyword argument when calling array #25922

ngoldbaum commented Mar 3, 2024

ngoldbaum commented Mar 3, 2024 •

edited

Loading

oscarbenjamin commented Mar 4, 2024

charris commented Mar 4, 2024

ngoldbaum commented Mar 4, 2024

ngoldbaum commented Mar 4, 2024

ngoldbaum commented Mar 4, 2024

ngoldbaum commented Mar 4, 2024

charris commented Mar 4, 2024

rgommers commented Mar 4, 2024

mtsokol Mar 4, 2024

mtsokol Mar 4, 2024

jorisvandenbossche Mar 5, 2024

rgommers Mar 5, 2024

jorisvandenbossche Mar 5, 2024

BUG: fix handling of copy keyword argument when calling __array__ #25922

BUG: fix handling of copy keyword argument when calling __array__ #25922

Conversation

ngoldbaum commented Mar 3, 2024

ngoldbaum commented Mar 3, 2024 • edited Loading

oscarbenjamin commented Mar 4, 2024

charris commented Mar 4, 2024

ngoldbaum commented Mar 4, 2024

ngoldbaum commented Mar 4, 2024

ngoldbaum commented Mar 4, 2024

ngoldbaum commented Mar 4, 2024

charris commented Mar 4, 2024

rgommers commented Mar 4, 2024

mtsokol Mar 4, 2024

Choose a reason for hiding this comment

mtsokol Mar 4, 2024

Choose a reason for hiding this comment

jorisvandenbossche Mar 5, 2024

Choose a reason for hiding this comment

rgommers Mar 5, 2024

Choose a reason for hiding this comment

jorisvandenbossche Mar 5, 2024

Choose a reason for hiding this comment

BUG: fix handling of copy keyword argument when calling array #25922

BUG: fix handling of copy keyword argument when calling array #25922

ngoldbaum commented Mar 3, 2024 •

edited

Loading