BUG,ENH: Fix internal `__array_wrap__` for direct calls #27807

seberg · Nov 21, 2024

Since adding return_scalar as am argument, the array-wrap implementations were slightly wrong when that argument was actually passed and the function called directly.

NumPy itself rarely (or never) did so for our builtin types now so that was not a problem within NumPy.

Further, the scalar version was completely broken, converting to scalar even when such a conversion was impossible.

As explained in the code. For array subclasses we NEVER want to convert to scalar by default. The subclass must make that choice explicitly.
(There are plenty of tests for this behavior.)

Closes gh-27783

Since adding `return_scalar` as am argument, the array-wrap implementations were slightly wrong when that argument was actually passed and the function called directly. NumPy itself rarely (or never) did so for our builtin types now so that was not a problem within NumPy. Further, the scalar version was completely broken, converting to scalar even when such a conversion was impossible. As explained in the code. For array subclasses we NEVER want to convert to scalar by default. The subclass must make that choice explicitly. (There are plenty of tests for this behavior.)

numpy/_core/src/multiarray/methods.c

seberg · Nov 21, 2024

@mhvk maybe you can have a quick glance over. I think it should be straight forward.

This should be back-portable, but not sure I think it is important enough to worry about it.
(Happy to delete the release note again, especially if we want to backport.)

mhvk

This looks good, and my comments are mostly about the phrasing of comments.

Only slightly larger item is whether there are already explicit tests for the case that self is a subclass and the item to be wrapped is the same subclass.

numpy/_core/src/multiarray/methods.c

mhvk · Nov 21, 2024

numpy/_core/src/multiarray/methods.c

@@ -892,24 +892,24 @@ static PyObject *
 array_wraparray(PyArrayObject *self, PyObject *args)
 {
    PyArrayObject *arr;
-    PyObject *obj;
+    PyObject *context_ignored = NULL;


Had to do a slight double take of whether this was a flag for whether the context would be ignored. How about just

PyObject *UNUSED = NULL; # for the context argument

mhvk · Nov 21, 2024

numpy/_core/src/multiarray/methods.c


+    /*
+     * Subclasses must implement `__array_wrap__` with all the arguments.


I found this a bit hard to parse. How about

* If the array does not have our own type, we always wrap, even * if `return_scalar` is set, so that we preserve the (presumably * important) subclass information. * If the type matches, we honor `return_scalar`, passing through * PyArray_Return to convert any array with ndim=0 to scalar.

Though perhaps even better to just move the comment up, replacing the new comment on l.897?

mhvk · Nov 21, 2024

numpy/_core/src/multiarray/scalartypes.c.src

@@ -2035,29 +2036,33 @@ gentype_getarray(PyObject *scalar, PyObject *args)
    return ret;
 }

-static char doc_sc_wraparray[] = "sc.__array_wrap__(obj) return scalar from array";
+static char doc_sc_wraparray[] = "Array wrap to implementation for scalar types";


delete "to" - and maybe still spell out the dunder method for easier grepping, i.e., "__array_wrap__ implementation for scalar types"

mhvk · Nov 21, 2024

numpy/_core/src/multiarray/scalartypes.c.src


+/*
+ * Array wrap for scalars, returning a scalar again preferentially.


Maybe

* __array_wrap__ for scalars, returning a scalar if possible.

mhvk · Nov 21, 2024

numpy/_core/src/multiarray/scalartypes.c.src

    PyArrayObject *arr;
+    PyObject *context_ignored = NULL;
+    /* return_scalar should be passed, but it was a scalar so prefer scalar */


/* return_scalar should be passed, but we're scalar, so we prefer scalars by default */

mhvk · Nov 21, 2024

numpy/_core/src/multiarray/methods.c

-                PyArray_FLAGS(arr), (PyObject *)self, obj);
+                PyArray_FLAGS(arr), (PyObject *)self, (PyObject *)arr);
+    }
+    else if (return_scalar) {


Absolute nitpick, but for similarity with the scalar case, perhaps

Py_INCREF(arr) if (return_scalar) { ...

Since the previous clause had a return, we don't need an else if strictly.

Though arguably less clear, so feel free to ignore (really, just because I thought I saw a difference in ref counting when reviewing).

mhvk · Nov 21, 2024

numpy/_core/tests/test_arrayobject.py

+    # * NumPy returns scalars, if `return_scalar` is passed as True to allow
+    #   manual calls to `arr.__array_wrap__` to do the right thing.
+
+    class MyArr(np.ndarray):


Should one also test a subclass that does not override __array_wrap__? Or is that done elsewhere?

mhvk · Nov 21, 2024

numpy/_core/tests/test_arrayobject.py

+        arr = arr.view(MyArr)
+
+    arr0d = np.array(3, dtype=np.int8)
+    # Third argument not passed, None, or True "decays" to scalar.


Copy & paste error from the scalar test? For an array, only True causes decay to scalar. Maybe just

# With third argument True, ndarray allows "decay" to scalar.

mhvk · Nov 21, 2024

numpy/_core/tests/test_arrayobject.py

+    if subclass:
+        arr = arr.view(MyArr)
+
+    arr0d = np.array(3, dtype=np.int8)


What if arr0d is a subclass too? As written, I think that causes decay to a scalar (if arr is a subclass).

True, but I don't think it should ever be a subclass! I.e. that would defeat the purpose of wrapping (converting everything to plain arrays)?
(Technically, subclasses are accepted here. But in practice, I think we must assume that the original subclass is meaningless, because we must return our subclass.)

Ah, sorry, I misunderstood. While it is probably true that this shouldn't happen, I agree that I should anticipate it and fix the branch (return of scalar must only happens if self is a base-class array)

…ixes

seberg · Nov 21, 2024

Thanks for the good spot, pushed a fix and hopefully made the comments/docs a bit clearer. It is slightly annoying because we have to very strictly check for the 0-d case.

(I am ignoring that scalar.__array_wrap__(subclass) returns the subclass. I think that is OK and strictly speaking I think the caller would be incorrect anyway.)

mhvk

One very small comment left, but feel free to ignore.

mhvk · Nov 21, 2024

numpy/_core/src/multiarray/methods.c

-                        "can only be called with ndarray object");
-        return NULL;
+
+    if (return_scalar && Py_TYPE(self) == &PyArray_Type && PyArray_NDIM(arr) == 0) {


the NDIM part can in principle be removed - PyArray_Return does that already. Maybe slightly better? (If going that route, adjust the comment to # Return scalar if NDIM==0.

Yeah, that is what I had... But then my test fails, because I somewhat prefer returning an ndarray rather than a subclass, and PyArray_Return won't do that.

Yeah, that is what I had... But then my test fails, because I somewhat prefer returning an ndarray rather than a subclass, and PyArray_Return won't do that.

OK, makes sense now!

seberg · Nov 22, 2024

I think I'll put this in then, thanks for the review @mhvk and spotting the subclass issue!.
Happy to follow-up if necessary.

github-actions bot added the 00 - Bug label Nov 21, 2024

seberg force-pushed the gentype-arrwrap branch from 2a172da to af1fcaa Compare November 21, 2024 12:40

seberg commented Nov 21, 2024

View reviewed changes

numpy/_core/src/multiarray/methods.c Show resolved Hide resolved

mhvk reviewed Nov 21, 2024

View reviewed changes

BUG: Ensure cast to self in ndarray.__array_wrap__ and other review f…

d46c61e

…ixes

mhvk approved these changes Nov 21, 2024

View reviewed changes

seberg merged commit 16b210c into numpy:main Nov 22, 2024
69 checks passed

seberg deleted the gentype-arrwrap branch November 22, 2024 08:13


		/*
		* Subclasses must implement `__array_wrap__` with all the arguments.


		/*
		* Array wrap for scalars, returning a scalar again preferentially.

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

Uh oh!

BUG,ENH: Fix internal __array_wrap__ for direct calls #27807

BUG,ENH: Fix internal __array_wrap__ for direct calls #27807

Uh oh!

Conversation

seberg commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

seberg commented Nov 21, 2024

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seberg commented Nov 21, 2024

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seberg commented Nov 22, 2024

Uh oh!

Uh oh!

Uh oh!

BUG,ENH: Fix internal `__array_wrap__` for direct calls #27807

BUG,ENH: Fix internal `__array_wrap__` for direct calls #27807

seberg commented Nov 21, 2024 •

edited

Loading