Repository navigation
Decide on what the resolution rules for __op__/__rop__/__numpy_ufunc__ actually are #5844
Description
There is a complex set of questions around how to handle method resolution in the presence of __numpy_ufunc__. Currently in master is an extremely complicated set of rules that isn't documented and that I don't actually understand (see #5748 for the latest set of changes to this), so it's kinda hard to know whether they are correct, but I suspect not. And this is a blocker for 1.10, b/c whatever we release in 1.10 will be set in stone forever.
I strongly feel that we cannot include __numpy_ufunc__ in a release without at least having a document somewhere describing what the actual dispatch rules are. I hope that doesn't mean we have to defer __numpy_ufunc__ for another release, but if it does then it does.
AFAICT this is how a op b dispatch works for ndarrays, BEFORE __numpy_ufunc__ (i.e., this is how 1.9 works):
- First Python uses the subclass rule to decide whether to invoke
a.__op__(b)orb.__rop__(a). So in the case where one of these objects is a proper subclass of the other, that object always gets to do absolutely anything, so that's fine. The interesting cases are the ones where neither is a proper subclass of the other (either because it's like,matrix + masked array, or because it's likendarray + scipy.sparse). So without loss of generality, let's focus on the case where Python callsa.__op__(b), andais either an instance ofndarrayor else an instance of a subclass ofndarraywhich has not overridden__op__, i.e. we're gettingndarray.__op__(a, b). ndarray.__op__has the following logic (seePyArray_GenericBinaryFunctioninnumber.c):- If
bis not anndarrayat all (even a subclass), andbhas a higher__array_priority__thana, then we returnNotImplementedand let control pass tob.__rop__(a). - Otherwise, we call
np.op(a, b)and let the ufunc machinery take over.
- If
np.op(a, b)does the following (seePyUFunc_GenericFunction,PyUFunc_GeneralizedFunction, inufunc_object.c, and alsoufunc_generic_callwhich converts-2return values from the previous intoNotImplementedso you have to audit their whole call stack):- If
bis not anndarray, and callingnp.array(b)returns an object array (presumably because coercion failed... though I guess this could also be hit ifb.__array__()return an object array or something), ANDbhas a higher__array_priority__thana, andbhas an__rop__method, then returnNotImplemented. - If any of our arrays contain structured dtypes or strings, and there are no special struct ufunc loops registered, but not if any of our arrays contain objects, then return
NotImplemented. (This is buried inget_ufunc_arguments, search forreturn -2.) - Otherwise we return the actual ufunc result.
- If
Now, my suggestion is that the way we would EVENTUALLY like this to look is:
- First, Python uses the subclass rule to decide whether to invoke
a.__op__(b)orb.__rop__(a). As above, let's assume that it invokesndarray.__op__(a, b). ndarray.__op__(a, b)callsnp.op(a, b)(which in turn invokes all the standard ufunc stuff, including__numpy_ufunc__resolution).- There is no step 3.
I submit that it is obvious that IF we can make this work, then it is obviously the ideal outcome, because it is the simplest possible solution. But is it too simple? To determine this we have to answer two questions: (1) Will it adequately address all the relevant use cases? (2) Can we get there from here?
So let's compare the current rules to my dream rules.
First, we observe that everything that currently happens inside the ufunc machinery looks like it's totally wrong. The first check can only be triggered if b is a non-ndarray that has a higher __array_priority__ (among other things), but if we look above, we see that those conditions are sufficient to trigger the check in ndarray.__op__, so checking again at the ufunc level is redundant at best. And the second check is just incoherent nonsense AFAICT. The only reason to return NotImplemented is b/c you want to pass control to another __(r)op__ method, and there's no reason arrays containing structured dtypes in particular should somehow magically have different __(r)op__ methods available than other arrays. So we can just get rid of all the ufunc stuff immediately, great.
That leaves the __array_priority__ stuff. We have two problems here: we can't just drop this immediately b/c of backcompat issues, and we need to have some way to continue to support all the use cases that this currently supports. The first problem is just a matter of having a deprecation period. For the second, observe that a class which defines a __numpy_ufunc__ method gets complete control over what any ufunc call does, so it has almost as much power as a class that currently sets __array_priority__. The only additional power that __array_priority__ currently gives you is that it lets you distinguish between e.g. a call to ndarray.__add(a, b) versus a call to np.add(a, b). So the only code that really loses out from my proposed change is code which wants a + b and add(a, b) to do different things.
AFAIK in the entire history of numpy there is only one situation where this power has been used on purpose: the definition of matrix classes where a * b is matmul, but np.multiply(a, b) is elmul. And we've all agreed that such classes should be deprecated and eventually phased out (cite: PEP 465).
So, I conclude that EVENTUALLY my dream rules should work great. The only problem is that we need some temporary compromises to get us from here to there. Therefore, I propose we use the following dispatch rules in numpy 1.10, with the goal of moving to my "dream rules" in some future version:
- First, Python uses the subclass rule to decide whether to invoke
a.__op__(b)orb.__rop__(a). As above, let's assume that it invokesndarray.__op__(a, b). ndarray.__op__(a, b)does the following:- If
bdoes not define__numpy_ufunc__and is not anndarrayat all (even a subclass), andbhas a higher__array_priority__thana, then we issue a deprecation warning and returnNotImplementedand let control pass tob.__rop__(a). (bolded parts are changes compared to the current behaviour) - If
__op__is__mul__andb->tp_class->tp_name.startswith("scipy.sparse."), then returnNotImplemented. (This rule is necessary in addition to the above, becausescipy.sparsehas already made a release containing__numpy_ufunc__methods, so the exception above doesn't apply.) - Otherwise, we call
np.op(a, b)and let the ufunc machinery take over.
- If
I believe that this is adequate to covers all practical use cases for the current dispatch machinery, and gives us a clean path to better dispatch machinery in the future.
The main alternative proposal is Pauli's, which involves a very complicated check (I won't try to summarize here, see this comment and following code). The goal of that approach is to continue supporting classes where a + b and add(a, b) do different things. I don't think that keeping substantial additional complexity around indefinitely is worth it in order to support functionality that no-one has ever found a use for except in one very specific case (overriding __mul__), and where we generally agree that that one specific case should be phased out as possible.
I would very much appreciate feedback from scipy.sparse and astropy in particular on whether the above covers all their concerns.
Activity
pv commentedon May 6, 2015
Hi, thanks for picking this up. I agree 100% this is an issue that has
to be resolved.
.
A clarification on the above writeup: when
__numpy_ufunc__is present,the dispatch mechanism, as it is currently implemented, completely skips
all of the legacy logic inside ufuncs that you outline above, and also
disregards
__array_priority__. The only logic that comes into play isthe Python binop step, plus the co-operative behavior explicitly coded
in number.c:needs_right_binop_forward. After that, command passes either
to numpy_ufunc, or to the right-hand operation of the other object.
.
The purpose for scrapping the ufunc-returns-NotImplemented and
array_priority logic is that as shown by experience, it doesn't work so
well, and moreover is already fairly complicated.
.
The motivation for the binop dispatch changes made together with
numpy_ufunc is the following: suppose we did not have
numpy_ufunc at all. How should the dispatch logic work so that
things make sense? (The only role numpy_ufunc currently has in the
dispatch logic, is that its presence turns off the old logic, and
switches on the new one.)
.
The criticism for the simpler dispatch logic above is the following:
what happens if the other object does not have numpy_ufunc present?
Numpy ufuncs will happily turn many things into object arrays, and will
unconditionally gobble up ndarray subclasses (potentially triggering
array_wrap etc.).
.
The second criticism is this: what is the point in replacing Python's
binop dispatch mechanism by a second layer of dispatch, which works in
the same way? Won't we run into the same problems in the second layer
that we ran in the first layer? (Note that numpy_ufunc still has a
logically independent purpose, as it enables overriding np.multiply and
all other ufunc ops, not just binops.) The answer to the second question
might be "no", but I don't see that from the top of my head.
pv commentedon May 6, 2015
The second point is: scipy.sparse does not need the new binop logic.
It does not have ndarray subclasses, and
__array_priority__will workfine for it (provided inplace ops are made to respect it). scipy.sparse has
always used array_priority to ensure its binops get a chance to run.
.
Note that that it works for inplace ops in the absence of
__numpy_ufunc__currently is because of the array_priority check, which is done inside
an ufunc called from the in-place binop invoked by ndarray. This check would
need to be moved to occur earlier (inside the ndarray binop) to make mul
be consistent with imul even when numpy_ufunc is present.
njsmith commentedon May 6, 2015
Right, the above writeup doesn't even attempt to describe the dispatch mechanism that currently exists in master (except briefly in the very last paragraph), just because I'm trying to focus on what we've previously released, and what we want to release next, and what's in master might or might not match either of those :-).
Right, we are 100% in agreement here.
I guess I don't understand why we should consider this hypothetical situation. We do have
__numpy_ufunc__, and we need it regardless of what happens at the binop layer, and in most cases binop dispatch will be followed by__numpy_ufunc__dispatch regardless. So to me the question is: given that we have__numpy_ufunc__, what should happen at the binop layer? Having two complicated dispatch systems is strictly worse than having just one, and as I argue above,__numpy_ufunc__is actually sufficient for everything you might want to do at the binop layer anyway. (Esp., keep in mind: whatever we do at the binop layer, will have to be replicated by every other array-like class, b/c otherwise consider what happens if I want to write some new, say, distributed array class that knows how to interoperate with both ndarrys and scipy.sparse arrays. Now whatever I need from ndarray's binop dispatch system, I also need to exist in scipy.sparse's binop dispatch system. So this is much much easier if we can tell people, when you implement an array-like class just dodef __add__(self, other): return np.add(self, other), etc., and your array-like will interoperate in a predictable way with every other array-like.)I'd prefer that
__numpy_ufunc__not play any role in the binop dispatch, and you can see in my "dream rules" that it doesn't. In my proposal, the only reason it affects the binop dispatch is because we need some signal to disable the__array_priority__deprecation warning for classes that no longer depend on the deprecated functionality.If you don't override how something works, then you get the default behavior, yes. I guess I don't see why that's a problem, or what you would expect to happen instead...? For now, anyone who has overridden
__op__methods will continue to see them called via the somewhat byzantine__array_priority__route. In the future, they should override__numpy_ufunc__instead.Not sure what you mean about ndarray subclasses -- subclasses are totally unaffected by all this stuff, b/c Python gives them total control over binop dispatch regardless of what we do.
We're not replacing Python's binop dispatch -- it still does what it does. And like you say, we have independent reasons to add
__numpy_ufunc__dispatch, so we necessarily have at least 2 layers of dispatch. This discussion is about whether we want to have yet another custom binop dispatch system in between those -- a third layer.I guess the argument here is really that there's something inadequate about
__numpy_ufunc__? ("Won't we run into the same problems...")The problem we run into in the first layer (Python's built-in dispatch) is that according to Python, what you're supposed to do is to try performing the operation, and then if you can't because you don't know what to do with the other object, you give up and let it try. But like you alluded to above, numpy has this "never say die" attitude where it will find some way to try and make
ndarray + other_objoradd(ndarray, other_obj)work, even if it's massively suboptimal (like calling__array__on a sparse array to convert it to dense form, or doing some weird object dtype thing). So we end up in this very sticky situation where we want to give the other object a chance to override the operation unconditionally, BUT if the other object doesn't want to then there's a good chance that we can go ahead and finish the operation after all via some fallback logic (coercing stuff to ndarray etc.). There's no way to express that in Python's built-in dispatch, b/c we have no good way to look at an object and know whether it can perform the operation ahead of time -- e.g., you can't just check whether there's an__add__method, b/clisthas an__add__method but you don't want to use it when doingndarray + list.But, this problem doesn't arise for
__numpy_ufunc__, b/c only objects that actually know how to deal with ndarrays have it defined :-). So we can get away with the rule that any object that has__numpy_ufunc__defined has opted-out of receiving fallback handling. (And if you really do want fallback handling it's pretty easy for a__numpy_ufunc__method to coerceselfto an ndarray manually and then retry the operation, which basically gives you fallback handling after all.) So I think__numpy_ufunc__does not run into the problems that we have with Python's built-in binop handling. Or am I missing something?pv commentedon May 6, 2015
As it appears to me, the problem is at the binop level, so it should be solved on that level, so that we don't throw away Python's binop mechanism along with the bathwater. That ndarrays directly call ufuncs, regardless of whether the operation can (for some meaning of "can") be done, renders the normal Python mechanism used to deal with binops inoperable. I don't think taking a "numpy exceptionalist" approach of using our own dispatch mechanism, working in pretty much the same way as the existing one, is completely necessary for a solution of the present problem.
The reason it does in the rules in master is to preserve legacy backward compatibility, exactly a you propose above.
However, note that (in the above proposal) the presence of numpy_ufunc still has a rather important effect --- it turns off the default ndarray binop cast-other-to-ndarray behavior. I think being able to do this is the key element in solving the binop problem, as it's present in all of the solutions proposed.
Yes, but I don't think numpy_ufunc solves this particular problem.
scipy.sparse.binop should return NotImplemented when it doesn't recognize the other object, and the execution then falls back to the other object as per normal Python binop rules. This should work fine.
However, if scipy.sparse has a cast-other-to-ndarray-and-gobble-it-up fallback behavior in the binop, it will run against the same problem that we currently have in numpy binops. However, exactly the same problem also arises in the numpy_ufunc hook, so it appears we have just shuffled the issue around.
Your suggested solution of cast-self-to-ndarray-and-then-try-again should also work with Python binops. The difference is then between
np.multiply(np.asarray(self), other)oroperator.mul(np.asarray(self), other)(modulo fallback to a cast-other-to-ndarray fallback if TypeError). In either case, the implementation needs to be written to be cooperative.Note that Python will not call the rhs op for binops between ndarray subclasses that are not subclasses of each other (the common situation). If they don't override all of the greedy behavior of ndarray default binop, interaction between them will again run into problems.
I think the thing is that the goal is also achievable with a suitably written numpy.ndarray.binop, and I think this is the more desirable way to go, because it doesn't throw away Python's standard mechanism of binop negotiation. Of course, both approaches require that all classes that want to interoperate have to write their binop/numpy_ufunc mechanisms in a certain way, to avoid the cast-other-to-ndarray behavior that is at the root of the problems.
It seems all of the solutions above, including array_priority (although it is was not consistently implemented), boil down to selectively disabling the default cast-other-to-ndarray behavior of the ndarray binop. The proposal above does that when numpy_ufunc is present, in which case it continues the method negotiation in numpy_ufunc; Python's own mechanism is skipped in all cases.
tl;dr; I don't like the idea of throwing away Python's own binop mechanism and replacing it with our own, essentially identical system. If a simpler approach is necessary, I'd rather go for custom attribute(s) in
otherfor disabling the ndarray default fallback behavior.mhvk commentedon May 6, 2015
Yes, good to have! Suggestion: would it make sense to make a wiki page in this so the documentation of the current and future API gets written as we go?
I will make one comment on arrays with added information like units, from my experience working on
Quantity,MaskedArray, and a newVariableclass. I've found it most useful to think of all of those as container classes, which add a single aspect to a set of numbers (a unit, array of masks, and array of uncertainties, resp.). In particular, while I might have felt differently when I started onQuantity, I now no longer feel it makes much sense to bring units deeper inside (as in adtype), though it may be useful to have ufuncs that do two calculations in one go (say scale*a + b).More generally, for all those classes
__numpy ufunc__is a great solution, as it allows on to strip off a given additional property, run the ufunc, allow it to call the__numpy_ufunc__on the next operand, etc. So far, it seems fine if there is no additional ordering mechanism, as long as if an operand has a__numpy_ufunc__it will always get called.mhvk commentedon May 6, 2015
Now read the thread more carefully: My sense would be (and I think we all agree) that we leave the present behaviour as unchanged as possible, and try to ensure
__numpy_ufunc__will get us to dream world.But we need to define more clearly what it means to "let the ufunc machinery take over". Could it be simpler than it is now? In particular, should the very first thing in a
ufuncjust be the equivalent of:(Here,
ufuncis just the present machinery, i.e., one that does not look at__numpy_ufunc__at all.)The thing that this removes is that no longer (https://github.com/numpy/numpy/blob/master/numpy/core/src/multiarray/number.c#L121):
Right now, the trial to implement the above prioritisation caused the problem in #4815 that
<Q-subclass> + <Q>yields aTypeError(to understand why, see @pv's #4815 (comment)). In this respect, reverting #5748 and using instead the solution I proposed is closer to the above simpleminded scheme. But what exactly makes the above prioritisation necessary?pv commentedon May 6, 2015
shoyer commentedon May 6, 2015
I am certainly inspired by @mhvk's vision of composable ndarray objects. I would love to be able to write new ndarray-like objects that add a single feature and all work well together, like lazy computation (e.g.,
dask.array) or labeled arrays (e.g.,pandas.Seriesorxray.DataArray).I think
__numpy_ufunc__and @njsmith's proposal here is better than the current system using__array_priority__, but I won't be 100% sure this covers my use cases until I try it.One thing that would be extremely helpful is some sort of "best practices" recipe for how one should implement binary operations on an ndarray-like object to make use of the numpy machinery. Let me give this a shot, just to see if I understand what I've read so far:
mhvk commentedon May 6, 2015
Separately, on non-ndarray objects having
__array_priority__: the main reason I used this is to ensurendarrayufuncs don't do their nefarious gobbling of anything into an object array. Indeed, this is an aspect ofndarraythat I really do not like, to try to make everything into an array (even though it has obvious benefits for python numbers). In this respect, I do like very much that I can just define__numpy_ufunc__ = Noneand be sure that, e.g.,<array> * <my-non-array>will eventually get to my__r<op>__methods.Example:
Probably should document this wonderful behaviour!
pv commentedon May 6, 2015
@shoyer: the system currently in master is not using array_priority
either at all (when numpy_ufunc is present).
.
@mvhk: that does not look like the intended use of numpy_ufunc. In the
proposal above, radd would not get called.
mhvk commentedon May 6, 2015
@pv - I see your point that in @njsmith's most simple scheme, the
__r<op>__functions will only be called by the CPython rule: if the RHS is a strict subclass of the LHS. But I guess I still do not understand why this would pose a general problem and/or why for specific cases where it does one cannot write a proper__numpy_ufunc__that takes care.EDIT: I meant of course that the
__r<op>__rules will only be called before theself.__<op>__rules by the CPython logic.mhvk commentedon May 6, 2015
@pv - Please don't remove it ;-)
But above I omitted my first trial, which is silly, as it is more obvious it should work:
The logic in the simple scheme would be (for
<ndarray> + B()):self.__add__(other), which isndarray.__add__(self, other);ndarray.__add__callsnp.add(self, other);np.addcallsother.__numpy_ufunc__;other.__numpy_ufunc__returnsNotImplemented, which is passed down toCPython;CPythoncallsother.__radd__(self), just as one would have hoped.mhvk commentedon May 6, 2015
@shoyer - I think your example is good, except that I would hope you would not need to define
__add__, etc., unless they do something more interesting than callingnp.add-- the standardndarraymethods should "just" do the right thing...And of course your
__numpy_ufunc__will have to do something with whatever it is that sets your class apart from anndarray. With multiple container elements, this may not be so trivial, especially if perhaps one would like to expose all class properties (say, have<masked-array> * <quantity>return aMaskedQuantityobject). But that is beyond the present discussion.njsmith commentedon May 6, 2015
That isn't how numpy_ufunc dispatch works, though -- remember that as
far as numpy_ufunc is concerned, there is no binop, the user may have
just done something like np.add(a, b), and np.add is not supposed to return
NotImemented under any circunstance. So the way numpy_ufunc is defined,
if you return NotImplemented from numpy_ufunc then you still won't get
any fallback behavior: the only two possibilities are that either some
other argument's numpy_ufunc will step in to handle the operation, or
else the operation will fail b/c no-one knows how to handle it. (Just like
what happens when add and radd both return NotImplemented.) I know
working on ndarray for subclasses for so long has warped your mind to the
point where using magic attributes to tweak the behavior of complex
fallback chains seems natural and obvious, but really we are trying to keep
things simpler than that :-).
(Also note that you want to support np.add(a, b) and for it to do the
same thing as +, so implementing a fancy radd instead of a fancy
numpy_ufunc is a waste of time anyway -- the latter is strictly more
useful.)
On May 6, 2015 12:28 PM, "Marten van Kerkwijk" [email protected]
wrote:
345 remaining items
BUG/ENH: Switch to simplified __array_ufunc__/binop interaction
BUG/ENH: Switch to simplified __array_ufunc__/binop interaction
BUG/ENH: Switch to simplified __array_ufunc__/binop interaction
BUG/ENH: Switch to simplified __array_ufunc__/binop interaction
BUG/ENH: Switch to simplified __array_ufunc__/binop interaction
BUG/ENH: Switch to simplified __array_ufunc__/binop interaction
BUG/ENH: Switch to simplified __array_ufunc__/binop interaction
eric-wieser commentedon Apr 28, 2017
Is this resolved by the merge of #8247?
shoyer commentedon Apr 28, 2017
@eric-wieser Yes, I think so, though note my follow-up in #9014
BUG/ENH: Switch to simplified __array_ufunc__/binop interaction
seberg commentedon Jul 14, 2020
I know its been a long time, but @njsmith and others. I just realized we do binop stuff in our scalar code. Why is that? We should be able to assume that scalars always defer to any array-like, no? Any array-like should be able to score our scalars. There could be some stranger things about when to defer to user defined scalars as opposed to going to ufunc directly.
I am willing to bet we can just defer in that case as well though? Removing all "binop_override.h" from the scalar math code entirely does not cause any test failures for one thing (although I suppose it might then go into ufuncs when it doesn't have to necessarily).
mattip commentedon Jul 14, 2020
I think the issue is speed. I wonder if there is a way to fast-path even more ufunc cases so this wouldn't matter.
seberg commentedon Jul 14, 2020
Yeah, we have to fall back to the ufunc commonly, and before we fall back to the ufunc, we have to check
__array_priority__, if scalars were scalars and not array-scalars that would not matter, butscalar + [1, 2, 3]actually works currently (and likely has to).However, it does not really make sense that we do the check before doing the scalar logic. We only have to do that binop check before falling back to ufuncs.
seberg commentedon Jul 14, 2020
So, I do think that we are just making scalars unnecessarily slow currently though. We need to defer once we reach the "use the array method (even though this is a scalar)" stage. That stage already does the check in either case though, so it seems we are doing the check up to 3 times if we end up going into the array branch. So I think we can remove the case completely from the scalar code.
If scalars fall back to the generic scalar code paths, these already just fall back to the array code path. So the only danger would be a scalar subclass implementing
__array_ufunc__. I will be so bold and claim that in any case where a scalar is a non-numpy scalar is involved, we should always defer though (which may cause in turn fallback to the full ufunc/ndarray slot path)