BUG: Pass non-None outputs to `__array_prepare` and `array_wrap__` #10919

eric-wieser · 2018-04-17T07:03:02Z

Fixes gh-10450.

This is the alternative to #10470 and #10488.

I've made these changes on top of one another, so the commit diffs are largely meaningless (will squash before merging)

The effect here is that all of these will pass args=(in1, in2) to __array_prepare__ and __array_wrap__:

ufunc(in1, in2) - unchanged
ufunc(in1, in2, None) - previously:
- prepare: (in1, in2, None)
- wrap: (in1, in2, None)
ufunc(in1, in2, out=None) - previously:
- prepare: (in1, in2, None)
ufunc(in1, in2, out=(None,)) - previously
- prepare: (in1, in2, (None,))

Whereas these will pass args=(in1, in2, out);

ufunc(in1, in2, out) - unchanged
ufunc(in1, in2, out=out) - previously
- wrap: (in1, in2)
ufunc(in1, in2, out=(out,)) - previously
- prepare: not called on out at all!
- wrap: (in1, in2)

mhvk · 2018-04-17T20:40:02Z

Overall, I think this does the right thing and it looks good. My main question is whether it is really all that helpful to introduce the new struct with in and out attributes. It would seem that if full_args is simply a tuple of length either nin or nin+nout, things would work even easier as it then just is the correct arr_prep_args (and the one nice consequence, checks for args.out == NULL, could just be replaced with size checks).

Whether or not the new code is worth may depend mostly on where you see it going: as you note, there is substantial refactoring still to be done, and if you think the structure will make that much easier, then perhaps it is worth keeping it.

eric-wieser · 2018-04-18T02:56:25Z

This structure makes it more suitable for use with __array_ufunc__, so I'd be inclined to keep it.

eric-wieser · 2018-04-18T02:58:23Z

numpy/core/src/umath/ufunc_object.c

@mhvk: The separation between in and out makes this a little more transparent (no idx_offset or start_idx)

True, but at the cost of a new function that combines in and out to remake arr_prep_args; just correcting the implementation of that would have a much smaller footprint.

That combination had to be added somewhere - part of the original bug was that only *args was being used, rather than a normalized args struct. Either we combine them at the call site, or we do it during the parse step.

Since we treat in and out differently in a bunch of places, it seems most useful to keep them separate till the last minute.

eric-wieser · 2018-04-18T02:59:28Z

numpy/core/tests/test_umath.py

Perhaps I should leave this test as it was - this was inherited from the earlier implementations

eric-wieser · 2018-04-20T05:35:57Z

Alright, squashed

eric-wieser · 2018-04-20T06:07:47Z

And now with much clearer tests, a comparison to the old behavior, and a couple new test cases

mhvk

Looks good to me! Only one suggestion remaining, but at it is purely a suggested short-cut, I'll approve now.

mhvk · 2018-04-20T13:11:18Z

numpy/core/src/umath/ufunc_object.c

Couldn't one just do

full_args->in = PyTuple_Slice(args, 0, nin);

Yeah, I see no reason why that wouldn't work

Ah, I remember why - PyTuple_Slice does a tonne of extra checks we don't need:

https://github.com/python/cpython/blob/master/Objects/tupleobject.c#L400-L440

Given that ufunc overhead seems to be something that comes up a lot, I though that this was worth optimizing.

I think the overhead is negligible compared to other things (like just looking up a method...) -- after all, it is just a few checks of ints -- and one has to weigh the overhead against the advantage of clarify, but I do not really mind either way. If you don't change it, maybe add a comment? (saying that we do what PyTuple_Slice would do, but without the overhead of its checks?)

Yes, adding a comment seems reasonable

I take it back, PyTuple_GetSlice removes the need for my optimization above too - so I'll switch

eric-wieser · 2018-04-21T22:39:00Z

Updated with PyTuple_GetSlice and a few extra comments in the source

mhvk · 2018-04-22T14:40:54Z

OK, the slice indeed also had the logic for the full slice in place, so now makes it quite a bit shorter, nice. Sadly, it needs a rebase now...

…repare__ or __array_wrap__ arguments This fixes numpygh-10450. The effect here is that all of these will pass `args=(in1, in2)` to `__array_prepare__` and `__array_wrap__`: * `ufunc(in1, in2)` - unchanged * `ufunc(in1, in2, None)` - previously: * prepare: `(in1, in2, None)` * wrap: `(in1, in2, None)` * `ufunc(in1, in2, out=None)` - previously: * prepare: `(in1, in2, None)` * `ufunc(in1, in2, out=(None,))` - previously * prepare: `(in1, in2, (None,))` Whereas these will pass `args=(in1, in2, out)`; * `ufunc(in1, in2, out)` - unchanged * `ufunc(in1, in2, out=out)` - previously * wrap: `(in1, in2)` * `ufunc(in1, in2, out=(out,))` - previously * prepare: not called on out at all! * wrap: `(in1, in2)`

eric-wieser · 2018-04-22T19:41:27Z

Rebased.

mattip · 2018-04-22T19:53:48Z

numpy/core/src/umath/ufunc_object.c

+make_full_arg_tuple(
+        ufunc_full_args *full_args,
+        npy_intp nin, npy_intp nout,
+        PyObject *args, PyObject *kwds)


shouldn't the output arguments be last in the declaration?

Putting them first matches memcpy(dst, src, n) and f(dst, args...) more closely resembles dst = f(args...)

I slightly wondered why the routine doesn't just return full_args (and NULL if it fails).

(Beyond that, @mattip, are you OK with the current implementation? My sense is that it is ready to go in.)

It is an improvement, from here refactoring will be easier

mattip · 2018-04-22T19:57:54Z

numpy/core/src/umath/ufunc_object.c

-        Py_INCREF(args);
-        return args;
+    /* This should have been checked by the caller */
+    assert(nin <= nargs && nargs <= nin + nout);


assert will be ignored in release

That's fine, I don't think this can ever happen

mattip · 2018-04-22T20:00:07Z

numpy/core/src/umath/ufunc_object.c

+    out_kwd = kwds ? PyDict_GetItem(kwds, npy_um_str_out) : NULL;
+
+    if (out_kwd != NULL) {
+        assert(nargs == nin);


Is there a check elsewhere? assert will be ignored in release

Yep, this check already happens in the first round of argument parsing

eric-wieser · 2018-04-26T06:24:27Z

@mhvk: Anything else needed here?

eric-wieser · 2018-04-26T06:30:21Z

I'm hoping this might make fixing #9022 easier

mhvk · 2018-04-26T15:48:14Z

OK, let's get it in!

Small thing missed in numpygh-10919.

eric-wieser added 00 - Bug component: numpy._core labels Apr 17, 2018

eric-wieser requested a review from mhvk April 17, 2018 07:03

eric-wieser force-pushed the __array_wrap__-non-None-outputs branch from e4b92a9 to 71e9258 Compare April 17, 2018 07:40

eric-wieser commented Apr 18, 2018

View reviewed changes

eric-wieser force-pushed the __array_wrap__-non-None-outputs branch from 8a0866f to 765fd45 Compare April 20, 2018 05:35

eric-wieser force-pushed the __array_wrap__-non-None-outputs branch from 765fd45 to 5d8b135 Compare April 20, 2018 06:07

mhvk approved these changes Apr 20, 2018

View reviewed changes

eric-wieser force-pushed the __array_wrap__-non-None-outputs branch from 5d8b135 to 665087f Compare April 21, 2018 03:20

eric-wieser force-pushed the __array_wrap__-non-None-outputs branch from 665087f to 40868df Compare April 22, 2018 19:41

mattip reviewed Apr 22, 2018

View reviewed changes

mhvk merged commit dadfb54 into numpy:master Apr 26, 2018

This was referenced Apr 26, 2018

BUG: Always include outputs in the arg tuple passed to __array_wrap__/__array_prepare__ #10470

Closed

BUG: Never pass output arguments to __array_wrap__ or __array_prepare__ #10488

Closed

mhvk added a commit to mhvk/numpy that referenced this pull request Jun 2, 2018

MAINT: ensure we do not create unnecessary tuples for outputs

b9e62d1

Small thing missed in numpygh-10919.

mhvk mentioned this pull request Jun 2, 2018

MAINT: ensure we do not create unnecessary tuples for outputs #11231

Merged

eric-wieser mentioned this pull request Jun 10, 2018

BUG: Fix segfault on failing __array_wrap__ #11296

Merged

charris changed the title ~~BUG: Pass non-None outputs to __array_prepare__ and __array_wrap__~~ BUG: Pass non-None outputs to __array_prepare__ and __array_wrap__ Jun 16, 2018

Uh oh!

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

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

Uh oh!

Conversation

eric-wieser commented Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Apr 17, 2018

Uh oh!

eric-wieser commented Apr 18, 2018

Uh oh!

eric-wieser Apr 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Apr 20, 2018

Uh oh!

eric-wieser commented Apr 20, 2018

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!

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 Apr 21, 2018

Uh oh!

mhvk commented Apr 22, 2018

Uh oh!

eric-wieser commented Apr 22, 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 Apr 26, 2018

Uh oh!

eric-wieser commented Apr 26, 2018

Uh oh!

mhvk commented Apr 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

BUG: Pass non-None outputs to `__array_prepare` and `array_wrap__` #10919

BUG: Pass non-None outputs to `__array_prepare` and `array_wrap__` #10919

eric-wieser commented Apr 17, 2018 •

edited

Loading

eric-wieser Apr 18, 2018 •

edited

Loading