BUG: core: ensure __r*__ has precedence over __numpy_ufunc__ #3856
Conversation
|
Yeah, the documented rule is that all the More generally it's not really clear to me what the motivation for this PR is? The way things work right now, we already have a well-defined mechanism for adjudicating between |
|
The point is to replace I'd believe that " The point of array_priority was previously only in ufunc output and input type determination, with some right-hand special cases hacked in. This part of the mechanism is completely disabled by the more Python-like approach now in numpy_ufunc. As far as ndarray subclasses are concerned, the changes in this PR are not needed, as Python works out the call order correctly itself. (EDIT: Except for the in-place ops --- I'm less sure whether what I did to them here is the correct thing. EDIT2: also, operations concerning different ndarray subclasses in which the other is not a subclass of the other probably also don't work correctly without these changes; I doubt this case worked out correctly with array_priority) The remaining issue is in non-ndarray subclasses. A similar binop special case as to what I add in this PR was added a couple of months ago in 4441bdd for array_priority --- also that is not in the ufunc code. (EDIT2: similar issue occurs AFAICS for ndarray subclasses in which one is not a subclass of the other --- I think the changes in that commit don't fix the behavior for this case) However, that change did not yet completely fix
EDIT3 This PR fixes the above four issues. It might be possible to fix them also by reworking the |
|
@pv Needs rebase. |
|
Rebased. |
| return 0; | ||
| } | ||
| if (!inplace_op && PyType_IsSubtype(Py_TYPE(other), Py_TYPE(self)) | ||
| || !PyArray_Check(self)) { |
|
LGTM modulo a nitpick. |
Add a special case to the implementation of ndarray.__mul__ et al. that refuses to work on other objects that are not ndarray subclasses and implement both __numpy_ufunc__ and __r*__. This way, execution passes first to the custom __r*__ method, which makes it possible to have e.g. __mul__ and np.multiply do different things. Additionally, disable one __array_priority__ special case handling when also __numpy_ufunc__ is defined.
|
Fix'd. What about @njsmith's objection that this is duplicating a similar thing as array_priority? My view on the situation is that |
|
@pv That has been my impression, although I haven't looked deeply into it. I think array priority works for some things, it did OK for the Polynomial classes, but many want to have more functionality than that and end up subclassing ndarray, mistakenly IMHO. Ndarray was not really designed as a virtual base class. So I think this helps with that. If it looks a bit of a kludge in some parts, I think that is unavoidable. The upshot is, I'm trusting your judgement on this one ;) |
BUG: core: ensure __r*__ has precedence over __numpy_ufunc__
|
Too bad this doesn't help with #3908 ;) |
Add a special case to the implementation of
ndarray.__mul__et al. that refuses to work on other objects that are not ndarray subclasses and implement both__numpy_ufunc__and__r*__.This way, execution passes first to the custom
__r*__method, which makes it possible to have e.g.__mul__and np.multiply do different things.Additionally, disable one
__array_priority__special case handling when also__numpy_ufunc__is defined (which may have been the reason why__rmul__continued to work in scipy.sparse matrices).Addresses gh-3812
Feedback welcome --- I think this solves the issue without being too magical, but YMMV