BUG: don't silence `__array_wrap__` errors in `ufunc.reduce` #9022

eric-wieser · 2017-04-29T01:52:16Z

Classes that declared __array_wrap__(self, obj, context), with no default argument for context, would find that this method was never called by reduce
Any errors that happen inside __array_wrap__ were silenced, and the result downcast to ndarray

eric-wieser · 2017-04-29T09:49:00Z

numpy/lib/tests/test_ufunclike.py

@@ -53,7 +53,8 @@ def __new__(cls, data, metadata=None):
                return res

            def __array_wrap__(self, obj, context=None):
-                obj.metadata = self.metadata
+                if isinstance(obj, MyArray):
+                    obj.metadata = self.metadata


This was previously crashing, because obj was ndarray, and did not have the metadata property.

I think there's a bigger issue here in that __array_prepare__ is not called either, but with __array_ufunc__ approaching release, I don't think we need to fix that any more

charris · 2017-05-11T17:22:44Z

numpy/core/src/umath/ufunc_object.c

        }
-        else {
+        else if (wrap != NULL) {
+            PyObject *res = PyObject_CallFunctionObjArgs(


Would almost prefer putting the initialization fo res on another line just to have space to put it all on a single line.

charris · 2017-05-11T17:23:29Z

numpy/core/src/umath/ufunc_object.c

+        else if (wrap != NULL) {
+            PyObject *res = PyObject_CallFunctionObjArgs(
+                wrap, ret, Py_None, NULL);
+            /* Handle __array_wrap__ that does not accept a context argument */


Blank lines after declarations is preferred.

charris · 2017-05-11T17:24:07Z

numpy/core/src/umath/ufunc_object.c

+            if (res == NULL && PyErr_ExceptionMatches(PyExc_TypeError)) {
+                PyErr_Clear();
+                res = PyObject_CallFunctionObjArgs(
+                    wrap, ret, NULL);


This all should fit on one line I think.

Again, copied from the other location. I guess I'll fix both

charris · 2017-05-11T17:26:05Z

numpy/core/src/umath/ufunc_object.c

+    else {
+        PyObject *wrap;
+        _find_array_wrap(args, kwds, &wrap, 1, 1);
+


I'd move this blank line up to after the declaration.

Extra blank line above else. I'm curious if the wrap here matters, the return will already have happened if out is not NULL.

NVM, I see that this is to wrap the return when out is missing.

charris · 2017-05-11T17:27:55Z

numpy/core/src/umath/ufunc_object.c

@@ -68,6 +68,9 @@
 /********************/

 /* ---------------------------------------------------------------- */
+static void
+_find_array_wrap(PyObject *args, PyObject *kwds,
+                PyObject **output_wrap, int nin, int nout);


Needs one more space of indentation.

Just copied from the definition, I think - I'll fix it there too

charris · 2017-05-11T18:05:50Z

Couple of style nits. Reading the ufunc code makes my head ache...

charris · 2017-05-30T18:03:48Z

Be good to finish this up.

eric-wieser · 2017-05-30T18:49:16Z

I'll take a look in the next few days once my schedule clears

eric-wieser · 2017-06-01T12:51:48Z

numpy/core/src/umath/ufunc_object.c

-            PyErr_Clear();
+    else {
+        PyObject *wrap;
+        _find_array_wrap(args, kwds, &wrap, 1, 1);


Turns out this doesn't work, as it interprets the axis argument as an out argument

eric-wieser · 2018-04-27T05:25:23Z

Overhauled, with the first two trivial cleanup commits extracted to #10988

eric-wieser · 2018-06-09T07:49:59Z

I'll see if I can revive this this weekend - seems my latest fixups segfault...

eric-wieser · 2018-06-11T00:15:41Z

Hopefully fixed by #11296

charris · 2018-06-11T03:07:37Z

numpy/core/src/umath/ufunc_object.c

-        return (PyObject *)ret;
-    }
+    /* Wrap and return the output */
+    {


I still don't like free floating blocks :)

Would you prefer if(true) {? :)

I could extract this to another _wrap_reduction_output helper function, but I think that starts to obscure the goal here.

Usually my goal with these floating blocks is to limit the scope of the variables, to make it clear where they should be used. C99 will remove most of the reason for my use of them

charris · 2018-06-11T03:10:18Z

numpy/core/src/umath/ufunc_object.c

-        else if (res == Py_None) {
-            Py_DECREF(res);
+        if (out != NULL) {
+            full_args.out = PyTuple_Pack(1, out);


Always one output in reductions?

Yep, reductions only apply to binary input unary output ufuncs

charris · 2018-06-11T03:35:53Z

numpy/core/src/umath/ufunc_object.c

+        return res;
+    }
+fail:
+    Py_DECREF(obj);


Don't need to Py_DECREF(wrap)? I see it is done in the one place above, just want to make sure everything that jumps to here has taken care of it.

charris · 2018-06-11T03:36:17Z

numpy/core/src/umath/ufunc_object.c

+            py_context = Py_BuildValue("OOi",
+                context->ufunc, args_tup, context->out_i);
+            Py_DECREF(args_tup);
+            if (py_context == NULL) {


What about wrap?

Good catch, thanks

charris · 2018-06-11T03:36:48Z

numpy/core/src/umath/ufunc_object.c

+            PyObject *args_tup;
+            /* Call the method with appropriate context */
+            args_tup = _get_wrap_prepare_args(context->args);
+            if (args_tup == NULL) {


What about wrap?

eric-wieser · 2018-06-11T06:25:09Z

Ok, fixed the missing decref on wrap.

Unfortunately wrap is still rarely leaked on error in the normal ufunc.__call__ case, but that was true before this patch - and I'd prefer to address it in a future PR.

mhvk · 2018-06-11T15:55:55Z

I'm sorry I'm late too this, but I'm worried about the change in behaviour for reductions. Is it needed to wrap even output that is provided or output that has the same type as the input? Since we don't pass in a context here, an implementation of __array_wrap__ can do exactly nothing except change the type - which in both those cases would be OK already. My sense would be to fix the bug but not the logic of when __array_wrap__ is called.

mhvk · 2018-06-11T15:56:53Z

numpy/core/src/umath/ufunc_object.c

@@ -87,6 +94,15 @@ _get_wrap_prepare_args(ufunc_full_args full_args) {
    }
 }

+static void


Maybe move the functions up instead?

I'd rather keep the diff smaller to make the blame easier

Fine to make it another commit; I find these prototypes before the actual code very confusing...

Done - I suppose in a standalone commit, a messy diff is fine if it comes with a commit message assuring nothing really changed.

eric-wieser · 2018-06-12T06:11:39Z

You're right, probably safer not to change the behavior here. I've modified it to keep the __array_wrap__ lookup logic the same, and just tweak the invocation logic.

eric-wieser · 2018-06-12T06:13:33Z

numpy/core/src/umath/ufunc_object.c

-            PyErr_Clear();
-        }
-        else if (res == Py_None) {
-            Py_DECREF(res);


I think this line was intended to mean a.__array_wrap__ is None, not a.__array_wrap__() is None

…ce as in ufunc.__call__ Previously they were silenced

…es are not needed The functions themselves have not changed, only moved

mhvk

Looks nice to me now; only one nit-pick argument which you should feel free to ignore (maybe it is even wrong).

mhvk · 2018-06-12T17:18:40Z

numpy/core/src/umath/ufunc_object.c

-        else {
-            PyObject *res;
-            PyObject *args_tup;
+        _ufunc_context context;


Maybe just _ufunc_context context = {ufunc, full_args, i}?

mhvk · 2018-06-13T01:26:36Z

numpy/core/src/umath/ufunc_object.c

-        else if (res == Py_None) {
-            Py_DECREF(res);
+    /* Wrap and return the output */
+    {


I must admit I find this open context ugly too - I'm trained to think I must be within a special block. I'd just move the PyObject *wrap to the top...

I put the comment before the block to make it clearer that there was no control structure missing. I think that the reduced number of local variable to keep track of is worth having a briefly misleading {. Also, when we adopt C99, cases like this can all be removed with a find and replace.

mhvk · 2018-06-13T01:28:24Z

numpy/core/src/umath/ufunc_object.c

-        else {
-            PyObject *res;
-            PyObject *args_tup;
+        context.ufunc = ufunc;


Now I think I've made the comment, but perhaps forgot to submit: maybe just define _ufunc_context context = {ufunc, full_args, i};? Or is that incorrect syntax? (My C remains quite hopeless...)

Sure, that seems reasonable - Definitely correct in some version of C, I think C90 will cope.

mhvk

Two nitpicks left (one shared by @charris...)

mhvk · 2018-06-13T12:55:39Z

numpy/core/src/umath/ufunc_object.c

@@ -4555,42 +4631,15 @@ ufunc_generic_call(PyUFuncObject *ufunc, PyObject *args, PyObject *kwds)
    /* wrap outputs */
    for (i = 0; i < ufunc->nout; i++) {
        int j = ufunc->nin+i;
-        PyObject *wrap = wraparr[i];
+        _ufunc_context context = {ufunc, full_args, i};


Appveyor complains about this line (https://ci.appveyor.com/project/charris/numpy/build/1.0.10809/job/3m1asbd6b1v588ro#L1894):
numpy\core\src\umath\ufunc_object.c(4634) : error C2440: 'initializing' : cannot convert from 'ufunc_full_args' to 'PyObject *'

Does look like the definition is indeed off.

Looks like a bug in the old msvc needed by python 2.7 and 3.4 - guess I'll revert

mattip · 2018-07-27T12:22:44Z

@mhvk is this ready to go?

mhvk · 2018-07-27T12:49:17Z

Yes, I approved this quite a while ago, and the nitpicks were discussed. Thanks for checking, @mattip!

eric-wieser added 00 - Bug component: numpy._core labels Apr 29, 2017

eric-wieser mentioned this pull request Apr 29, 2017

ufuncs return 0d arrays instead of scalars for ndarray subclasses #5819

Closed

eric-wieser changed the title ~~BUG: don't silence __array_wrap__ errors in reduce~~ BUG: don't silence __array_wrap__ errors in ufunc.reduce Apr 29, 2017

eric-wieser force-pushed the reduce-hides-error branch from 8affdad to fbb53d0 Compare April 29, 2017 09:47

eric-wieser commented Apr 29, 2017

View reviewed changes

charris reviewed May 11, 2017

View reviewed changes

eric-wieser force-pushed the reduce-hides-error branch from fbb53d0 to 2122172 Compare June 1, 2017 12:25

eric-wieser commented Jun 1, 2017

View reviewed changes

eric-wieser added the 55 - Needs work label Sep 13, 2017

This was referenced Apr 26, 2018

BUG: Pass non-None outputs to __array_prepare__ and __array_wrap__ #10919

Merged

MAINT: Small tidy-ups to ufunc_object.c #10988

Merged

eric-wieser force-pushed the reduce-hides-error branch from 2122172 to 8789582 Compare April 27, 2018 05:24

eric-wieser removed the 55 - Needs work label Apr 27, 2018

eric-wieser force-pushed the reduce-hides-error branch from 8789582 to 1d6d047 Compare April 27, 2018 06:59

eric-wieser changed the base branch from master to fwd_port_1.14.3_changelog May 14, 2018 15:42

eric-wieser changed the base branch from fwd_port_1.14.3_changelog to master May 14, 2018 15:43

eric-wieser mentioned this pull request Jun 10, 2018

MAINT: Extract a helper function to apply __array_wrap__ #11295

Closed

eric-wieser force-pushed the reduce-hides-error branch from 1d6d047 to a9b063f Compare June 11, 2018 00:13

charris reviewed Jun 11, 2018

View reviewed changes

MAINT: Extract a helper function to apply __array_wrap__

8eeab6d

eric-wieser force-pushed the reduce-hides-error branch 2 times, most recently from 7546bf7 to f25853a Compare June 11, 2018 06:23

mhvk reviewed Jun 11, 2018

View reviewed changes

eric-wieser force-pushed the reduce-hides-error branch from f25853a to ae51067 Compare June 12, 2018 06:10

eric-wieser commented Jun 12, 2018

View reviewed changes

eric-wieser force-pushed the reduce-hides-error branch 2 times, most recently from facd479 to 90d747a Compare June 12, 2018 07:58

eric-wieser added 2 commits June 12, 2018 09:06

BUG/MAINT: Handle errors in __array_wrap__ the same way in ufunc.redu…

29e68f6

…ce as in ufunc.__call__ Previously they were silenced

MAINT: Hoist helper functions to the top of the file so that prototyp…

e3b0a3f

…es are not needed The functions themselves have not changed, only moved

eric-wieser force-pushed the reduce-hides-error branch from 90d747a to e3b0a3f Compare June 12, 2018 16:08

mhvk approved these changes Jun 12, 2018

View reviewed changes

mhvk reviewed Jun 13, 2018

View reviewed changes

eric-wieser force-pushed the reduce-hides-error branch from 0751591 to e3b0a3f Compare June 13, 2018 17:51

mhvk merged commit 43bbeef into numpy:master Jul 27, 2018

charris changed the title ~~BUG: don't silence __array_wrap__ errors in ufunc.reduce~~ BUG: don't silence __array_wrap__ errors in ufunc.reduce Nov 10, 2018

Uh oh!

BUG: don't silence __array_wrap__ errors in ufunc.reduce #9022

BUG: don't silence __array_wrap__ errors in ufunc.reduce #9022

Uh oh!

Conversation

eric-wieser commented Apr 29, 2017

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!

charris commented May 11, 2017

Uh oh!

charris commented May 30, 2017

Uh oh!

eric-wieser commented May 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser commented Apr 27, 2018

Uh oh!

eric-wieser commented Jun 9, 2018

Uh oh!

eric-wieser commented Jun 11, 2018

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!

eric-wieser commented Jun 11, 2018

Uh oh!

mhvk commented Jun 11, 2018

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!

eric-wieser commented Jun 12, 2018

Uh oh!

Choose a reason for hiding this comment

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!

BUG: don't silence `__array_wrap__` errors in `ufunc.reduce` #9022

BUG: don't silence `__array_wrap__` errors in `ufunc.reduce` #9022

eric-wieser Jun 13, 2018 •

edited

Loading