BUG: fix for #4753 (out keyword with __numpy_ufunc__) #5566
Conversation
|
I haven't looked at the code yet, but your summary note has me scared and
|
|
I don't understand the meaning of this comment either. ufuncs do not break scoping, whether they are overridden or not? |
|
If I understand the meaning correctly, in the statement "... if we actually want c to contain the result like ..." the answer is that we certainly don't want this --- the non-overridden ufuncs do not do this sort of magic either. |
|
Basically I as confused about how this would work if |
|
Yeah, if kwarg["out"] were immutable then the correct thing to do would be
|
397845d to
7ec0347
Compare
|
I updated the tests when I realized what was going on. This is ready for review. |
|
Where is this at now? |
|
@cowlicks - just to add that I think from the perspective of a developer using this, having to deal with |
| if ((kwds)&& (PyDict_CheckExact(kwds))) { | ||
| obj = PyDict_GetItemString(kwds, "out"); | ||
| if (obj != NULL) { | ||
| if (PyObject_HasAttrString(obj, "__numpy_ufunc__")) { |
There was a problem hiding this comment.
This may be a microoptimization, but similarly to what you do in check_tuple_for_numpy_ufunc, you could skip searching the object's dictionary for the string if it is None or a strict ndarray or scalar.
There was a pr 8000 oblem hiding this comment.
Okay thanks @jaimefrio
| int nargs = PyTuple_GET_SIZE(args); | ||
| PyObject *obj; | ||
|
|
||
| for (; *i < (og_i + nargs); ++*i) { |
There was a problem hiding this comment.
Ewww. Instead of *i, use a local variable and assign its value on exit. Best rename the argument to something other than i also.
|
I will rebase and take a look at #5674 tomorrow and see what needs to change here. I think my original incorrect comments on this PR are just confusing everyone. So I'll make a new PR when I'm done and close this. |
|
Thanks @mhvk since that seems to be the general expectation. |
This address bug #4753
It should be noted that while we now check the
outkeyword arg for an overriding object, the third party__numpy_ufunc__method needs to implement its own breaking of function scoping to actually assign the result to whatever is inkwds['out']outside the function.So when we do
if we actually want
cto contain the result like:Then in
OverrideObject.__numpy_ufunc__you would have to accessc(which is out of scope) and set it as the result.To fix this I would probably have to integrate the override stuff more into ufunc argument processing. I'm not sure how to do this yet.