8000 `__array__` copy keyword changes incomplete · Issue #25916 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

__array__ copy keyword changes incomplete #25916

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
rgommers opened this issue Mar 2, 2024 · 9 comments · Fixed by #25922
Closed

__array__ copy keyword changes incomplete #25916

rgommers opened this issue Mar 2, 2024 · 9 comments · Fixed by #25922
Labels
06 - Regression component: numpy._core
Milestone
2.0.0 release

Comments

@rgommers
Copy link
Member
rgommers commented Mar 2, 2024

It's still very early in the morning and I may simply not be awake yet, but it looks to me like the __array__ changes in gh-25168 are incomplete and don't have a regression test for the old signature:

In [1]: import numpy as np

In [2]: class MyArr:
   ...:     def __init__(self, x):
   ...:         self._x = x
   ...:     def __array__(self, dtype=None):
   ...:         return self._x
   ...: 

In [3]: x = np.arange(3)

In [4]: np.asarray(x)
Out[4]: array([0, 1, 2])

In [5]: np.asarray(MyArr(x))
...
TypeError: MyArr.__array__() got an unexpected keyword argument 'copy'

In [6]: np.__version__
Out[6]: '2.0.0.dev0+git20240301.06b169b'

I didn't follow the review of the C code in detail, so I'm probably missing some details, but it doesn't look like the current implementation does what was intended. In particular:

  • only pass along the new copy keyword if it's not the default value of None, to have a smaller impact,
  • pass the actual True/False/None values; it now only passes False or None (never_copy ? Py_False : Py_None;),
  • when retrying without the new keyword, the dtype keyword should still be passed, so the code should pop copy and retry with kwargs rather than passing NULL.

It looks like the changes should be along these lines (incomplete):

$ git diff
diff --git a/numpy/_core/src/multiarray/ctors.c b/numpy/_core/src/multiarray/ctors.c
index 4b9c3add3f..897e141336 100644
--- a/numpy/_core/src/multiarray/ctors.c
+++ b/numpy/_core/src/multiarray/ctors.c
@@ -2442,26 +2442,46 @@ PyArray_FromArrayAttr_int(
 
     PyObject *copy = never_copy ? Py_False : Py_None;
     PyObject *kwargs = PyDict_New();
-    PyDict_SetItemString(kwargs, "copy", copy);
+
+    /*
+     * Only if the value of `copy` isn't the default one, we try to pass it
+     * along; for backwards compatibility we then retry if it fails because the
+     * signature of the __array__ method being called does not have `copy`.
+     */
+    int copy_passed = 0;
+    if (copy != Py_None) {
+        /* XXX: why can `copy` only be False or None, not True? */
+        copy_passed = 1;
+        PyDict_SetItemString(kwargs, "copy", copy);
+    }
     PyObject *args = descr != NULL ? PyTuple_Pack(1, descr) : PyTuple_New(0);
 
     new = PyObject_Call(array_meth, args, kwargs);
 
     if (PyErr_Occurred()) {
+        PyObject *errmsg_substr = PyUnicode_FromString(
+                "__array__() got an unexpected keyword argument 'copy'");
+        if (errmsg_substr == NULL) {
+            return NULL;
+        }
         PyObject *type, *value, *traceback;
         PyErr_Fetch(&type, &value, &traceback);
-        if (PyUnicode_Check(value) && PyUnicode_CompareWithASCIIString(value,
-                    "__array__() got an unexpected keyword argument 'copy'") == 0) {
+        if (PyUnicode_Check(value) && PyUnicode_Contains(value, errmsg_substr) > 0) {
             Py_DECREF(type);
             Py_XDECREF(value);
             Py_XDECREF(traceback);
+            Py_DECREF(errmsg_substr);
             if (PyErr_WarnEx(PyExc_UserWarning,
                              "__array__ should implement 'dtype' and 'copy' keywords", 1) < 0) {
                 return NULL;
             }
-            Py_SETREF(new, PyObject_Call(array_meth, args, NULL));
+            if (copy_passed) { /* try again */
+                PyDict_DelItemString(kwargs, "copy");
+                Py_SETREF(new, PyObject_Call(array_meth, args, kwargs));
+            }
         } else {
             PyErr_Restore(type, value, traceback);
+            Py_DECREF(errmsg_substr);
             return NULL;
         }
     }

Cc @mtsokol @ngoldbaum @seberg.
@rgommers rgommers added this to the 2.0.0 release milestone Mar 2, 2024
@rgommers rgommers mentioned this issue Mar 2, 2024
Merged
@oscarbenjamin
Copy link

This breaks a number of tests in sympy:

    def test_issue_14943():
        # Test that __array__ accepts the optional dtype argument
        try:
            from numpy import array
        except ImportError:
            skip('NumPy must be available to test creating matrices from ndarrays')
    
        M = Matrix([[1,2], [3,4]])
>       assert array(M, dtype=float).dtype.name == 'float64'
E       TypeError: MatrixBase.__array__() got an unexpected keyword argument 'copy'

sympy/matrices/tests/test_matrices.py:2967: TypeError

At the same time we are also seeing a segfault in CI with the numpy nightly wheels but I don't know if that is related to this (they both seem to happen at the same time):
https://github.com/sympy/sympy/actions/runs/8116952589/job/22188138475?pr=26294

https://groups.google.com/g/sympy/c/OAcu4xV-R7k

@oscarbenjamin
Copy link

we are also seeing a segfault in CI

I can't reproduce the segfault locally so it is only in CI for now...

@mattip
Copy link
Member
mattip commented Mar 3, 2024

At the same time we are also seeing a segfault in CI with the numpy nightly wheels

Can you try a CI run where you only use one of nightly numpy/nightly scipy to try to narrow it down a bit?

@seberg
Copy link
Member
seberg commented Mar 3, 2024 •
edited
Loading

Almost certainly related, so not sure it is worth digging. this is the test in question (where it segfaults):

def test_issue_14943():
    # Test that __array__ accepts the optional dtype argument
    try:
        from nu
8000
mpy import array
    except ImportError:
        skip('NumPy must be available to test creating matrices from ndarrays')

    M = Matrix([[1,2], [3,4]])
    assert array(M, dtype=float).dtype.name == 'float64'

@rgommers
Copy link
Member Author
rgommers commented Mar 3, 2024

Yes, the Py_SETREF(new, PyObject_Call(array_meth, args, NULL)); line seems to be problematic.

This was referenced Mar 3, 2024
Closed
Merged
@ngoldbaum ngoldbaum mentioned this issue Mar 3, 2024
Merged
@oscarbenjamin
Copy link

Thanks all. The sympy CI jobs are green now so it looks like it's working as expected.

If anything about __array__ should be changed on sympy's end let us know. Currently Matrix has:

class Matrix:
   def __array__(self, dtype=object):
        ...

We could add a copy parameter there but it can't do anything useful. There is no possibility of avoiding a "copy" because a sympy.Matrix needs to become a numpy.ndarray so it needs to be a newly created array.

Perhaps it should be something like:

class Matrix:
   def __array__(self, dtype=object, copy=None):
        if copy is not None and not copy:
            raise TypeError("Cannot implement copy=False when converting Matrix to ndarray")
        ...

The dev docs suggest that the signature should have copy=None as a parameter:
https://numpy.org/devdocs/user/basics.interoperability.html#the-array-method

Is the intention that not having the copy parameter will lead to deprecation warnings in future? I don't see any warnings in CI right now.

@rgommers
Copy link
Member 8000 Author
rgommers commented Mar 4, 2024

That signature and logic look right to me.

Is the intention that not having the copy parameter will lead to deprecation warnings in future? I don't see any warnings in CI right now.

Indeed. Right now it's a UserWarning, not a deprecation. It'll probably be a while before we deprecate it (a couple of releases at least I guess).

@oscarbenjamin oscarbenjamin mentioned this issue Mar 4, 2024
Closed
@charris
Copy link
Member
charris commented Mar 4, 2024

It'll probably be a while before we deprecate it

I was thinking a couple of years :)

@ngoldbaum
Copy link
Member

Also you’ll only see the warning if you explicitly pass copy=False, the keyword is not passed for the default copy=None case for better back compat in the common case.

@ngoldbaum ngoldbaum mentioned this issue Mar 29, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
06 - Regression component: numpy._core
Projects
None yet
Milestone
2.0.0 release
Development

Successfully merging a pull request may close this issue.

BUG: fix handling of copy keyword argument when calling __array__ ngoldbaum/numpy
6 participants
@seberg @charris @rgommers @mattip @oscarbenjamin @ngoldbaum
0