-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
BUG: __array_ufunc__= None
-> TypeError
#9014
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
92f5670
to
9ef5891
Compare
doc/neps/ufunc-overrides.rst
Outdated
equivalent to returning :obj:`NotImplemented`, so a :exc:`TypeError` is | ||
raised. In option (2), we pass directly to ``arr.__array_ufunc__``, | ||
which will return :obj:`NotImplemented`, which we catch. | ||
in term immediately raises :exc:`TypeError`, because one of its operands |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
term <- turn
doc/neps/ufunc-overrides.rst
Outdated
of NumPy but that also need to support binary arithmetic with NumPy arrays | ||
on older versions, we recommend using a backwards compatible variant of | ||
:class:`~numpy.lib.mixins.NDArrayOperatorsMixin` that we intend to provide | ||
in a separate repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard to recommend something that doesn't yet exist ;) Is there really a need to provide that in a separate repository? I can see the virtue of not depending on the NumPy release cycle, but I wonder how much that would matter in practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good question how necessary this really is. But we already have a test suite for NDArrayOperatorsMixin, so I think it would be pretty easy to write this backport.
numpy/core/src/umath/override.c
Outdated
@@ -342,6 +342,29 @@ PyUFunc_CheckOverride(PyUFuncObject *ufunc, char *method, | |||
} | |||
|
|||
/* | |||
* Raise an error if an argument disables ufuncs. | |||
*/ | |||
for (i = 0; i < noa; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems a shame not to make this happen in PyUFunc_WithOverride
. Maybe return -1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe even raise the error there and return -1 to indicate bail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that makes more sense.
@mhvk question for you: why do you check for |
@shoyer: "expected" is tricky here :-). Most Python users expect that methods are looked up by doing attribute access on the object, like... methods are. But (some) expert Python users will expect that In [1]: class Foo:
...: def __add__(self, other):
...: return "class"
...:
In [2]: f = Foo()
In [3]: f.__add__ = lambda other: "object"
In [4]: f + f
Out[4]: 'class'
In [5]: f.__add__(f)
Out[5]: 'object' I think this was originally partly an optimization and partly a hold-over from the whole C-level slots system that partly dates back to ancient days of Python when C classes and Python classes were totally different things, but it's standard enough now that PEPs regularly document it as part of the formal semantics for new features, e.g. check out the calls to OTOH I'm pretty sure numpy doesn't do this for other special attributes like One interesting question is whether or not doing the lookup directly on the class is faster – object attr lookup is fairly complicated, and this does skip some steps. And lookup speed for special attrs is something we've found worth optimizing in the past. |
@njsmith OK, fair enough. It seems totally reasonable to me to do the override lookup on the class for the sake of speed given that this check is in the inner loop of essentially every numpy operation. But in that case, we should document it, and also be consistent in |
The reason I made it look up As for then later looking it up on the object: I did consider just keeping the override itself instead of the object defining an override, but one still has to do the subclass check, so it didn't seem quite worth it. That said, I do think there is substantial room for optimization... |
Overall, I'm still not sure this is a good idea; see #9011 (comment) and the following comment. That said, much of the clarification of intent in the NEP is great, in particular that if |
#9021 is in. @mhvk I think it should be possible to relax this choice in the future if a need to do so turns up. My own sense is that if a class wants to be handled by subclasses only, it should use the python python ops or |
@charris - fair enough, it is easier to allow than to disallow later. |
__array_ufunc__= None
-> TypeError and update NEP
c39d19f
to
3c769e6
Compare
I have added a preliminary adjustments to the docs on |
__array_ufunc__= None
-> TypeError and update NEP__array_ufunc__= None
-> TypeError
@@ -49,11 +49,26 @@ has_non_default_array_ufunc(PyObject *obj) | |||
} | |||
|
|||
/* | |||
* Check whether an object sets __array_ufunc__ = None. | |||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might note that the existence of the attribute must be checked before calling this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -102,8 +99,7 @@ NumPy provides several hooks that classes can customize: | |||
|
|||
Alternatively, if ``obj.__array_ufunc__`` is set to :obj:`None`, then as a | |||
special case, special methods like ``ndarray.__add__`` will notice this | |||
and *unconditionally* return :obj:`NotImplemented`, so that Python will | |||
dispatch to ``obj.__radd__`` instead. This is useful if you want to define | |||
and *unconditionally* raise :exc:`TypeError`. This is useful if you want to | |||
a special object that interacts with arrays via binary operations, but |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"a special object" <- "create objects" and adjust singular verbs to plural.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Couple of minor suggestions for documentation, otherwise LGTM. |
Having read the revised NEP, I feel this is a fine approach (even if I still slightly prefer the current state), so OK to merge as far as I'm concerned. |
3c769e6
to
777534d
Compare
I fixed up my commits per @charris's review. I'll wait for CI to pass and then merge. |
Beat you to it. Thanks Stephan. |
Fixes #9011, by making
__array_ufunc__ = None
on any operand causeTypeError
to be raised.I have also taken the opportunity to update the NEP, adjusting the language for this change and also added a recommendation for how to implement arithmetic alongside
__array_ufunc__
(xref #8892). This made me realize an important practical difference between @mhvk's options (1) and (2): option (1) is viral, in the sense that other objects implementing arithmetic with your object also need to follow NumPy's rules.TODO: