Repository navigation
DOC: update binary-op / ufunc interactions and recommendations in ufunc overrides NEP - #9027
Conversation
6b9d342 to
490578e
Compare
| provides overrides for all binary operators with corresponding ufuncs. | ||
|
|
||
| implementation for :class:`ndarray` itself. We decided to require disabling | ||
| ufuncs to opt out to ensure that a class cannot define Ufuncs to return |
There was a problem hiding this comment.
Bit confusing, "ufuncs to opt out in order to ensure" would be clearer I think.
| ``-`` ``sub`` :func:`subtract` | ||
| ``*`` ``mul`` :func:`multiply` | ||
| ``/`` ``truediv`` :func:`true_divide` | ||
| (Python 3) |
There was a problem hiding this comment.
Would be better to put this on the line above, same below.
There was a problem hiding this comment.
Sphinx does a weird thing if I put it on one line, making the column very wide (I think this is basic on the longer length of the column header ======). So this version ends up looking better when rendered to HTML.
| ``&`` ``and_`` :func:`bitwise_and` | ||
| ``^`` ``xor_`` :func:`bitwise_xor` | ||
| ``|`` ``or_`` :func:`bitwise_or` | ||
| \ ``divmod`` :func:`floor_divide`, :func:`mod` [*]_ |
There was a problem hiding this comment.
I put in "NA" instead. I am concerned that "--" could be confused with an infix operator.
| ``^`` ``xor_`` :func:`bitwise_xor` | ||
| ``|`` ``or_`` :func:`bitwise_or` | ||
| \ ``divmod`` :func:`floor_divide`, :func:`mod` [*]_ | ||
| ``@`` ``matmul`` Not yet implemented |
There was a problem hiding this comment.
Maybe "Not yet implemented as a ufunc".
|
@shoyer - thanks for doing this. I'm happy with your changes. |
| implementation for :class:`ndarray` itself. We decided to require disabling | ||
| ufuncs to opt out to ensure that a class cannot define Ufuncs to return | ||
| different results than the corresponding binary operations (i.e., if | ||
| ufuncs to opt out in order to ensure that a class cannot define Ufuncs to |
There was a problem hiding this comment.
Hmm, now that I look at this in combination with "We decided to require disabling" I still can't figure out what it says ;) I think what this is trying to say is that ufuncs will raise TypeError when one of the arguments opts out in order to ensure ...
There was a problem hiding this comment.
I took another pass at this.
| disabling Ufuncs so a class cannot define a Ufuncs to return a different | ||
| result than the corresponding binary operations (i.e., if | ||
| ``np.add(x, y)`` is defined, it should match ``x + y``). Our goal was to | ||
| simply the dispatch logic for binary operations with NumPy arrays |
|
Looks good apart from the wrong word. At some point it could use a bit more polish but for now it is good enough. |
| - Otherwise, :class:`ndarray` unilaterally calls the corresponding Ufunc. | ||
| Ufuncs never return ``NotImplemented``, so **reflexive methods such | ||
| as** ``other.__rmul__`` **cannot be used to override arithmetic with | ||
| NumPy arrays if** ``__array_ufunc__`` **is set**. Instead, the resulting |
There was a problem hiding this comment.
maybe make it "set to any value besides None"?
| @@ -482,12 +506,12 @@ are not compatible, i.e., implementations should be something like:: | |||
|
|
|||
| # Option 1: call ufunc directly | |||
There was a problem hiding this comment.
BTW, feel free to declare this out-of-scope for this PR, but maybe we should drop the discussion of "option 2" from the NEP now that we've pretty much settled on "option 1" as recommended?
| binary arithmetic with :class:`ndarray` (i.e., they must either implement | ||
| ``__array_ufunc__`` or set it to :obj:`None`). We believe this is a good | ||
| thing, because it ensures the consistency of ufuncs and arithmetic on all | ||
| objects that support them. |
There was a problem hiding this comment.
Oh, I see, you added more discussion of option 1 vs 2 instead. That's also a plausible approach!
I probably wouldn't use "viral" for something we want to recommend... it's a pretty loaded word? Plus it kinda doesn't matter whether people want their classes to be "viral" or not, because this is what ndarray does, so to the extent it's viral they have to follow, and if they don't follow it's not so viral then is it...?
It might be simplest to just delete the option 2 stuff from the NEP; the history's always available in git. Or this is probably fine; honestly it's 11:55 pm and I don't really have brain to track all the details in this giant block of text and that's probably influencing my opinion...
There was a problem hiding this comment.
I think as long as the interface is provisional, we should keep both options; even for ndarray itself, we will want to optimize things and might well find it is easier in a slightly different way.
Clarify the role of `array_ufunc` in overriding Python numeric operators.
4 participants
Split off and extended from #9014
Adds:
__array_ufunc__ = None.NDArrayOperatorsMixin.Because we have listed authors on the NEP, I would like to make sure that every listed author approves of the document before we merge this change:
(Feel free to check off your name on this list if you have edit rights, or I will do so, after you've responded affirmatively in a comment.)