BUG: Always include outputs in the arg tuple passed to __array_wrap__/__array_prepare__ #10470
Conversation
eric-wieser
force-pushed
the
fix-__array_wrap__-out
branch
from
42f95ad to
30e98a8
Compare
numpy/core/tests/test_umath.py
Outdated
There was a problem hiding this comment.
I don't understand why this has changed. I think one should continue to suppress all-None outputs.
There was a problem hiding this comment.
All none outputs were not suppressed before when none was passed explicitly.
There was a problem hiding this comment.
But here you do not pass in None!
There was a problem hiding this comment.
Right, but it's important that passing the documented default behave the same as not passing it, whatever that behavior may be. I could get behind stripping all-none outputs too though - it just means users are far less likely to implement some-none outputs correctly
numpy/ma/core.py
Outdated
There was a problem hiding this comment.
So, this is my worry: if even in numpy, we need this fix for __array_wrap__, we are likely going to break other classes as well. Do any tests break if we remove the output arguments in __array_prepare__?
There was a problem hiding this comment.
Remember that this part of the patch stands alone as a bug fix for np.add(a, b, c) currently inheriting the mask of c.
Any classes that we break are necessarily already broken in some calling convention in 1.14.
Omitting output arguments is possibly safest simply since passing them in the first place is uncommon.
There was a problem hiding this comment.
Ah, yes, I forgot that __array_wrap__ does get them if put as a positional argument. I wonder if it makes more sense to do this fix in a separate PR, so we can disentangle it from the larger reworking (which could them remove this again if we go for no outputs at all).
There was a problem hiding this comment.
Yep, this seems like an uncontroversial fix - i'll make a separate patch for these lines (#10479)
eric-wieser
changed the title
|
^ that was a hack to make github register the first commit is already merged |
Fixes numpygh-10450. The previous behavior for `ufunc(..., out=arr)` is now applied to all calling conventions for `out` - namely, the arg tuple passed into `__array_wrap__` and `__array_prepare__` is `(in1, ..., inN, out1, ..., outN)`. This tuple will contain `None` for absent `out` argument, which is consistent with what already happened for `ufunc(..., out=None)`. This breaks one test that asserts the length of the args tuple is `2` for np.minimum(x, y). However, I'd argue that test was broken anyway, and it has been expanded to cover all cases.
eric-wieser
force-pushed
the
fix-__array_wrap__-out
branch
from
30e98a8 to
e7f85fd
Compare
|
#10919 replaces this PR. |
2 participants
We still parse the
outargument 3 times in every ufunc call (PyUFunc_Gener*Function,get_ufunc_arguments,ufunc_generic_call), but at least we now do it the same way each time. Actually reusing the result can wait for a maintenance PR.Fixes gh-10450.
The previous behavior for
ufunc(in1, in2, out_arr)is now applied to all calling conventions forout- namely, the arg tuple passed into__array_wrap__and__array_prepare__is(in1, ..., inN, out1, ..., outN). This tuple will containNonefor absentoutargument, which is consistent with what already happened forufunc(..., None).This breaks one test that asserts the length of the args tuple is
2for np.minimum(x, y).However, I'd argue that test was broken anyway, and it has been expanded to cover all cases.