From 9ef3687bc39f0a138b3569fe71c8e669e2cd8320 Mon Sep 17 00:00:00 2001 From: Pauli Virtanen Date: Thu, 24 Jul 2014 23:58:28 +0300 Subject: [PATCH 1/2] BUG: core: ensure binop execution uses ufuncs as fallback These changes only affect objects defining __numpy_ufunc__. Other objects use the __array_priority__ mechanism to decide whether NotImplemented should be returned or not. The binops previously returned NotImplemented even if other._r__ is ndarray.__r__, rather than trying to evaluate the result via ufuncs. This causes problems in binops of two objects that both subclass from ndarray and define __numpy_ufunc__. The solution added here makes the total logic as follows: def __binop__(self, other): if (hasattr(other, '__numpy_ufunc__') and not isinstance(other, self.__class__) and hasattr(other, '__rop__') and other.__class__.__rop__ is not self.__class__.__rop__): return NotImplemented return np.binop(self, other) --- numpy/core/src/multiarray/arrayobject.c | 18 ++++-- numpy/core/src/multiarray/number.c | 81 ++++++++++++++----------- 2 files changed, 59 insertions(+), 40 deletions(-) diff --git a/numpy/core/src/multiarray/arrayobject.c b/numpy/core/src/multiarray/arrayobject.c index d92522a9f83f..a68f16c3c02a 100644 --- a/numpy/core/src/multiarray/arrayobject.c +++ b/numpy/core/src/multiarray/arrayobject.c @@ -1276,7 +1276,8 @@ array_richcompare(PyArrayObject *self, PyObject *other, int cmp_op) switch (cmp_op) { case Py_LT: - if (needs_right_binop_forward(obj_self, other, "__gt__", 0)) { + if (needs_right_binop_forward(obj_self, other, "__gt__", 0) && + Py_TYPE(obj_self)->tp_richcompare != Py_TYPE(other)->tp_richcompare) { /* See discussion in number.c */ Py_INCREF(Py_NotImplemented); return Py_NotImplemented; @@ -1285,7 +1286,8 @@ array_richcompare(PyArrayObject *self, PyObject *other, int cmp_op) n_ops.less); break; case Py_LE: - if (needs_right_binop_forward(obj_self, other, "__ge__", 0)) { + if (needs_right_binop_forward(obj_self, other, "__ge__", 0) && + Py_TYPE(obj_self)->tp_richcompare != Py_TYPE(other)->tp_richcompare) { Py_INCREF(Py_NotImplemented); return Py_NotImplemented; } @@ -1301,7 +1303,8 @@ array_richcompare(PyArrayObject *self, PyObject *other, int cmp_op) Py_INCREF(Py_False); return Py_False; } - if (needs_right_binop_forward(obj_self, other, "__eq__", 0)) { + if (needs_right_binop_forward(obj_self, other, "__eq__", 0) && + Py_TYPE(obj_self)->tp_richcompare != Py_TYPE(other)->tp_richcompare) { Py_INCREF(Py_NotImplemented); return Py_NotImplemented; } @@ -1376,7 +1379,8 @@ array_richcompare(PyArrayObject *self, PyObject *other, int cmp_op) Py_INCREF(Py_True); return Py_True; } - if (needs_right_binop_forward(obj_self, other, "__ne__", 0)) { + if (needs_right_binop_forward(obj_self, other, "__ne__", 0) && + Py_TYPE(obj_self)->tp_richcompare != Py_TYPE(other)->tp_richcompare) { Py_INCREF(Py_NotImplemented); return Py_NotImplemented; } @@ -1438,7 +1442,8 @@ array_richcompare(PyArrayObject *self, PyObject *other, int cmp_op) } break; case Py_GT: - if (needs_right_binop_forward(obj_self, other, "__lt__", 0)) { + if (needs_right_binop_forward(obj_self, other, "__lt__", 0) && + Py_TYPE(obj_self)->tp_richcompare != Py_TYPE(other)->tp_richcompare) { Py_INCREF(Py_NotImplemented); return Py_NotImplemented; } @@ -1446,7 +1451,8 @@ array_richcompare(PyArrayObject *self, PyObject *other, int cmp_op) n_ops.greater); break; case Py_GE: - if (needs_right_binop_forward(obj_self, other, "__le__", 0)) { + if (needs_right_binop_forward(obj_self, other, "__le__", 0) && + Py_TYPE(obj_self)->tp_richcompare != Py_TYPE(other)->tp_richcompare) { Py_INCREF(Py_NotImplemented); return Py_NotImplemented; } diff --git a/numpy/core/src/multiarray/number.c b/numpy/core/src/multiarray/number.c index a26a93c1d3eb..22fc95b2ae95 100644 --- a/numpy/core/src/multiarray/number.c +++ b/numpy/core/src/multiarray/number.c @@ -109,6 +109,13 @@ has_ufunc_attr(PyObject * obj) { * [occurs if the other object is a strict subclass provided * the operation is not in-place] * + * An additional check is made in GIVE_UP_IF_HAS_RIGHT_BINOP macro below: + * + * (iv) other.__class__.__r*__ is not self.__class__.__r*__ + * + * This is needed, because CPython does not call __rmul__ if + * the tp_number slots of the two objects are the same. + * * This always prioritizes the __r*__ routines over __numpy_ufunc__, independent * of whether the other object is an ndarray subclass or not. */ @@ -144,13 +151,19 @@ needs_right_binop_forward(PyObject *self, PyObject *other, } } -#define GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, left_name, right_name, inplace) \ - do { \ - if (needs_right_binop_forward((PyObject *)m1, m2, right_name, \ - inplace)) { \ - Py_INCREF(Py_NotImplemented); \ - return Py_NotImplemented; \ - } \ +/* In pure-Python, SAME_SLOTS can be replaced by + getattr(m1, op_name) is getattr(m2, op_name) */ +#define SAME_SLOTS(m1, m2, slot_name) \ + (Py_TYPE(m1)->tp_as_number != NULL && Py_TYPE(m2)->tp_as_number != NULL && \ + Py_TYPE(m1)->tp_as_number->slot_name == Py_TYPE(m2)->tp_as_number->slot_name) + +#define GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, left_name, right_name, inplace, slot_name) \ + do { \ + if (needs_right_binop_forward((PyObject *)m1, m2, right_name, inplace) && \ + !SAME_SLOTS(m1, m2, slot_name)) { \ + Py_INCREF(Py_NotImplemented); \ + return Py_NotImplemented; \ + } \ } while (0) @@ -337,21 +350,21 @@ PyArray_GenericInplaceUnaryFunction(PyArrayObject *m1, PyObject *op) static PyObject * array_add(PyArrayObject *m1, PyObject *m2) { - GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__add__", "__radd__", 0); + GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__add__", "__radd__", 0, nb_add); return PyArray_GenericBinaryFunction(m1, m2, n_ops.add); } static PyObject * array_subtract(PyArrayObject *m1, PyObject *m2) { - GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__sub__", "__rsub__", 0); + GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__sub__", "__rsub__", 0, nb_subtract); return PyArray_GenericBinaryFunction(m1, m2, n_ops.subtract); } static PyObject * array_multiply(PyArrayObject *m1, PyObject *m2) { - GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__mul__", "__rmul__", 0); + GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__mul__", "__rmul__", 0, nb_multiply); return PyArray_GenericBinaryFunction(m1, m2, n_ops.multiply); } @@ -359,7 +372,7 @@ array_multiply(PyArrayObject *m1, PyObject *m2) static PyObject * array_divide(PyArrayObject *m1, PyObject *m2) { - GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__div__", "__rdiv__", 0); + GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__div__", "__rdiv__", 0, nb_divide); return PyArray_GenericBinaryFunction(m1, m2, n_ops.divide); } #endif @@ -367,7 +380,7 @@ array_divide(PyArrayObject *m1, PyObject *m2) static PyObject * array_remainder(PyArrayObject *m1, PyObject *m2) { - GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__mod__", "__rmod__", 0); + GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__mod__", "__rmod__", 0, nb_remainder); return PyArray_GenericBinaryFunction(m1, m2, n_ops.remainder); } @@ -531,7 +544,7 @@ array_power(PyArrayObject *a1, PyObject *o2, PyObject *NPY_UNUSED(modulo)) { /* modulo is ignored! */ PyObject *value; - GIVE_UP_IF_HAS_RIGHT_BINOP(a1, o2, "__pow__", "__rpow__", 0); + GIVE_UP_IF_HAS_RIGHT_BINOP(a1, o2, "__pow__", "__rpow__", 0, nb_power); value = fast_scalar_power(a1, o2, 0); if (!value) { value = PyArray_GenericBinaryFunction(a1, o2, n_ops.power); @@ -561,56 +574,56 @@ array_invert(PyArrayObject *m1) static PyObject * array_left_shift(PyArrayObject *m1, PyObject *m2) { - GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__lshift__", "__rlshift__", 0); + GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__lshift__", "__rlshift__", 0, nb_lshift); return PyArray_GenericBinaryFunction(m1, m2, n_ops.left_shift); } static PyObject * array_right_shift(PyArrayObject *m1, PyObject *m2) { - GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__rshift__", "__rrshift__", 0); + GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__rshift__", "__rrshift__", 0, nb_rshift); return PyArray_GenericBinaryFunction(m1, m2, n_ops.right_shift); } static PyObject * array_bitwise_and(PyArrayObject *m1, PyObject *m2) { - GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__and__", "__rand__", 0); + GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__and__", "__rand__", 0, nb_and); return PyArray_GenericBinaryFunction(m1, m2, n_ops.bitwise_and); } static PyObject * array_bitwise_or(PyArrayObject *m1, PyObject *m2) { - GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__or__", "__ror__", 0); + GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__or__", "__ror__", 0, nb_or); return PyArray_GenericBinaryFunction(m1, m2, n_ops.bitwise_or); } static PyObject * array_bitwise_xor(PyArrayObject *m1, PyObject *m2) { - GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__xor__", "__rxor__", 0); + GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__xor__", "__rxor__", 0, nb_xor); return PyArray_GenericBinaryFunction(m1, m2, n_ops.bitwise_xor); } static PyObject * array_inplace_add(PyArrayObject *m1, PyObject *m2) { - GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__iadd__", "__radd__", 1); + GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__iadd__", "__radd__", 1, nb_inplace_add); return PyArray_GenericInplaceBinaryFunction(m1, m2, n_ops.add); } static PyObject * array_inplace_subtract(PyArrayObject *m1, PyObject *m2) { - GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__isub__", "__rsub__", 1); + GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__isub__", "__rsub__", 1, nb_inplace_subtract); return PyArray_GenericInplaceBinaryFunction(m1, m2, n_ops.subtract); } static PyObject * array_inplace_multiply(PyArrayObject *m1, PyObject *m2) { - GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__imul__", "__rmul__", 1); + GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__imul__", "__rmul__", 1, nb_inplace_multiply); return PyArray_GenericInplaceBinaryFunction(m1, m2, n_ops.multiply); } @@ -618,7 +631,7 @@ array_inplace_multiply(PyArrayObject *m1, PyObject *m2) static PyObject * array_inplace_divide(PyArrayObject *m1, PyObject *m2) { - GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__idiv__", "__rdiv__", 1); + GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__idiv__", "__rdiv__", 1, nb_inplace_divide); return PyArray_GenericInplaceBinaryFunction(m1, m2, n_ops.divide); } #endif @@ -626,7 +639,7 @@ array_inplace_divide(PyArrayObject *m1, PyObject *m2) static PyObject * array_inplace_remainder(PyArrayObject *m1, PyObject *m2) { - GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__imod__", "__rmod__", 1); + GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__imod__", "__rmod__", 1, nb_inplace_remainder); return PyArray_GenericInplaceBinaryFunction(m1, m2, n_ops.remainder); } @@ -635,7 +648,7 @@ array_inplace_power(PyArrayObject *a1, PyObject *o2, PyObject *NPY_UNUSED(modulo { /* modulo is ignored! */ PyObject *value; - GIVE_UP_IF_HAS_RIGHT_BINOP(a1, o2, "__ipow__", "__rpow__", 1); + GIVE_UP_IF_HAS_RIGHT_BINOP(a1, o2, "__ipow__", "__rpow__", 1, nb_inplace_power); value = fast_scalar_power(a1, o2, 1); if (!value) { value = PyArray_GenericInplaceBinaryFunction(a1, o2, n_ops.power); @@ -646,56 +659,56 @@ array_inplace_power(PyArrayObject *a1, PyObject *o2, PyObject *NPY_UNUSED(modulo static PyObject * array_inplace_left_shift(PyArrayObject *m1, PyObject *m2) { - GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__ilshift__", "__rlshift__", 1); + GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__ilshift__", "__rlshift__", 1, nb_inplace_lshift); return PyArray_GenericInplaceBinaryFunction(m1, m2, n_ops.left_shift); } static PyObject * array_inplace_right_shift(PyArrayObject *m1, PyObject *m2) { - GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__irshift__", "__rrshift__", 1); + GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__irshift__", "__rrshift__", 1, nb_inplace_rshift); return PyArray_GenericInplaceBinaryFunction(m1, m2, n_ops.right_shift); } static PyObject * array_inplace_bitwise_and(PyArrayObject *m1, PyObject *m2) { - GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__iand__", "__rand__", 1); + GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__iand__", "__rand__", 1, nb_inplace_and); return PyArray_GenericInplaceBinaryFunction(m1, m2, n_ops.bitwise_and); } static PyObject * array_inplace_bitwise_or(PyArrayObject *m1, PyObject *m2) { - GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__ior__", "__ror__", 1); + GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__ior__", "__ror__", 1, nb_inplace_or); return PyArray_GenericInplaceBinaryFunction(m1, m2, n_ops.bitwise_or); } static PyObject * array_inplace_bitwise_xor(PyArrayObject *m1, PyObject *m2) { - GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__ixor__", "__rxor__", 1); + GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__ixor__", "__rxor__", 1, nb_inplace_xor); return PyArray_GenericInplaceBinaryFunction(m1, m2, n_ops.bitwise_xor); } static PyObject * array_floor_divide(PyArrayObject *m1, PyObject *m2) { - GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__floordiv__", "__rfloordiv__", 0); + GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__floordiv__", "__rfloordiv__", 0, nb_floor_divide); return PyArray_GenericBinaryFunction(m1, m2, n_ops.floor_divide); } static PyObject * array_true_divide(PyArrayObject *m1, PyObject *m2) { - GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__truediv__", "__rtruediv__", 0); + GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__truediv__", "__rtruediv__", 0, nb_true_divide); return PyArray_GenericBinaryFunction(m1, m2, n_ops.true_divide); } static PyObject * array_inplace_floor_divide(PyArrayObject *m1, PyObject *m2) { - GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__ifloordiv__", "__rfloordiv__", 1); + GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__ifloordiv__", "__rfloordiv__", 1, nb_inplace_floor_divide); return PyArray_GenericInplaceBinaryFunction(m1, m2, n_ops.floor_divide); } @@ -703,7 +716,7 @@ array_inplace_floor_divide(PyArrayObject *m1, PyObject *m2) static PyObject * array_inplace_true_divide(PyArrayObject *m1, PyObject *m2) { - GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__itruediv__", "__rtruediv__", 1); + GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, "__itruediv__", "__rtruediv__", 1, nb_inplace_true_divide); return PyArray_GenericInplaceBinaryFunction(m1, m2, n_ops.true_divide); } @@ -735,7 +748,7 @@ static PyObject * array_divmod(PyArrayObject *op1, PyObject *op2) { PyObject *divp, *modp, *result; - GIVE_UP_IF_HAS_RIGHT_BINOP(op1, op2, "__divmod__", "__rdivmod__", 0); + GIVE_UP_IF_HAS_RIGHT_BINOP(op1, op2, "__divmod__", "__rdivmod__", 0, nb_divmod); divp = array_floor_divide(op1, op2); if (divp == NULL) { From 4617e4cd487a30f6bc24a3b3a75e02b3a02066b3 Mon Sep 17 00:00:00 2001 From: Pauli Virtanen Date: Fri, 25 Jul 2014 00:20:53 +0300 Subject: [PATCH 2/2] BUG: core: inplace ops don't have corresponding rhs ops, so no need to check the slot --- numpy/core/src/multiarray/number.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/numpy/core/src/multiarray/number.c b/numpy/core/src/multiarray/number.c index 22fc95b2ae95..4f269261dbca 100644 --- a/numpy/core/src/multiarray/number.c +++ b/numpy/core/src/multiarray/number.c @@ -160,7 +160,7 @@ needs_right_binop_forward(PyObject *self, PyObject *other, #define GIVE_UP_IF_HAS_RIGHT_BINOP(m1, m2, left_name, right_name, inplace, slot_name) \ do { \ if (needs_right_binop_forward((PyObject *)m1, m2, right_name, inplace) && \ - !SAME_SLOTS(m1, m2, slot_name)) { \ + (inplace || !SAME_SLOTS(m1, m2, slot_name))) { \ Py_INCREF(Py_NotImplemented); \ return Py_NotImplemented; \ } \