ENH: Add NDArrayOperatorsMixin #19
Conversation
numpy/lib/mixins.py
Outdated
|
|
||
| Caveats: the rarely used `divmod` (``__divmod__``) and unary `+` | ||
| (``__pos__``) operators do not have corresponding ufuncs. Hence this mixin | ||
| passes the builtin functions ``divmod`` and ``operator.pos`` to |
There was a problem hiding this comment.
ENH: add a np.divmod ufunc? Is there an issue for this?
numpy/lib/tests/test_mixins.py
Outdated
| kwargs['out'] = tuple(x.value if isinstance(self, type(x)) else x | ||
| for x in out) | ||
| result = getattr(ufunc, method)(*inputs, **kwargs) | ||
| if isinstance(result, tuple): |
There was a problem hiding this comment.
Needs an early exit path here for method == 'at', else we wrap None by accident.
Also, although ufunc.nout == 1 can't be used for divmod here, we can be a bit stricter by making this check be type(result) is tuple.
numpy/lib/mixins.py
Outdated
| __rshift__, __rrshift__, __irshift__ = _numeric_methods(np.right_shift) | ||
| __and__, __rand__, __iand__ = _numeric_methods(np.logical_and) | ||
| __xor__, __rxor__, __ixor__ = _numeric_methods(np.logical_xor) | ||
| __or__, __ror__, __ior__ = _numeric_methods(np.logical_or) |
There was a problem hiding this comment.
This is all super nice!
I do wonder whether we can do this base class in C though, so that this logic isn't duplicated inside the numpy C code.
e8a53fc to
e995bca
Compare
|
At least one Travis-CI test is still failing on this branch: |
numpy/lib/tests/test_mixins.py
Outdated
|
|
||
| with warnings.catch_warnings(): | ||
| # ignore warnings from operator.div when run with python -3 | ||
| warnings.filterwarnings('ignore', 'classic int division', |
There was a problem hiding this comment.
This didn't work:
https://travis-ci.org/shoyer/numpy/jobs/221437434
https://travis-ci.org/shoyer/numpy/jobs/221437437
Any other ideas for how to ignore these warnings on Python 2?
numpy/lib/tests/test_mixins.py
Outdated
| # one return value | ||
| return type(self)(result) | ||
| else: | ||
| # no return return value, e.g., ufunc.at |
There was a problem hiding this comment.
The word return is repeated
This doesn't successfully distinguish between "the result is a scalar from an object array and it is None" and "this ufunc method never has a result". For instance, np.add.reduce([None]) returns None, but that should probably be wrapped for consistency with np.add.reduce([object()]).
I don't think there is any sensible implementation other than if method == 'at'
There was a problem hiding this comment.
I wonder if it's worth handling this at all, given that this helper function currently only exists for the benefit of testing arithmetic. But I guess we'll probably be linking to this in the docs as recommended practices, so it's probably better to be on the safe side...
numpy/lib/mixins.py
Outdated
| __add__, __radd__, __iadd__ = _numeric_methods(um.add) | ||
| __sub__, __rsub__, __isub__ = _numeric_methods(um.subtract) | ||
| __mul__, __rmul__, __imul__ = _numeric_methods(um.multiply) | ||
| __div__, __rdiv__, __idiv__ = _numeric_methods(um.divide) # Python 2 only |
There was a problem hiding this comment.
Shouldn't this be in a if sys.version_info.major < 2, or whatever name we give that in numpy? I don't think these methods have any place in code for 3
numpy/lib/tests/test_mixins.py
Outdated
| @@ -91,7 +91,7 @@ def check(result): | |||
| check(np.array(0) + ArrayLike(np.array(0))) | |||
|
|
|||
| def test_divmod(self): | |||
| # divmod is subtle: it's returns a tuple | |||
| # divmod is subtle: its returns a tuple | |||
|
@shoyer What is the status of this? |
|
Here's my TODO list:
|
|
Thanks @shoyer . |
|
@charris I trust you'll squash this into a single commit before merging numpy#8247 :) |
|
Nevermind, I see you already did that :) |
| except AttributeError: | ||
| pass | ||
| return self.__array_ufunc__(ufunc, '__call__', other, self) | ||
| return func |
There was a problem hiding this comment.
Would be nice if func.__name__ could be set appropriately. So something like _reflected_binary_method(np.multiply, 'mul'), and internally it produces the name '__r{}__'.format('mul')
There was a problem hiding this comment.
The mixin has been updated in the latest version, might want to review the current PR.
There was a problem hiding this comment.
I think I will merge the current state in any case as the process is getting unwieldy at this point. You can make a new PR after that which should be easy to review and merge.
This mixin class provides an easy way to implement arithmetic operators that defer to
__array_ufunc__likenumpy.ndarrayin non-ndarray subclasses.