Repository: cupy/cupy
Pull request: Disallow boolean `negative`
Close #692.

Problem statement:
Negative operation for a boolean array is forbidden in NumPy
In NumPy negative operation for a boolean array is forbidden.
```
>>> -numpy.array([True])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: The numpy boolean negative, the `-` operator, is not supported, use the `~` operator or the logical_not function instead.
>>> -cupy.array([True])
array([False], dtype=bool)
```

Hints from issue discussion:
Related to this issue, NumPy raises DeprecationWarning for boolean subtract operation, which causes test failure (via pytest-warning).

```
>>> x = np.array([True])
>>> x - x
__main__:1: DeprecationWarning: numpy boolean subtract, the `-` operator, is deprecated, use the bitwise_xor, the `^` operator, or the logical_xor function instead.
array([False], dtype=bool)
>>> x = cp.array([True])
>>> x - x
array([False], dtype=bool)
```

Confirmed with NumPy 1.9.0, and it will raise TypeError instead of DeprecationWarning in master branch of NumPy (maybe 1.14.0).

Test result follows. Note that test is not deterministic (combinations are randomly chosen), so it may not always happen.

```
self = <cupy_tests.core_tests.test_ndarray_elementwise_op.TestArrayElementwiseOp testMethod=test_array_reversed_sub>

    def test_array_reversed_sub(self):
>       self.check_array_reversed_op(operator.sub)

self       = <cupy_tests.core_tests.test_ndarray_elementwise_op.TestArrayElementwiseOp testMethod=test_array_reversed_sub>

tests/cupy_tests/core_tests/test_ndarray_elementwise_op.py:458: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
cupy/testing/helper.py:736: in test_func
    impl(self, *args, **kw_copy)
cupy/testing/helper.py:105: in test_func
    accept_error=accept_error)
cupy/testing/helper.py:49: in _check_cupy_numpy_error
    self.fail('Only numpy raises error\n\n' + numpy_tb)
E   AssertionError: Only numpy raises error
E   
E   Traceback (most recent call last):
E     File "/work/cupy/cupy/testing/helper.py", line 26, in _call_func
E       result = impl(self, *args, **kw)
E     File "/work/cupy/tests/cupy_tests/core_tests/test_ndarray_elementwise_op.py", line 452, in check_array_reversed_op
E       return op(a, b[::-1])
E   DeprecationWarning: numpy boolean subtract, the `-` operator, is deprecated, use the bitwise_xor, the `^` operator, or the logical_xor function instead.
```
In NumPy 1.9.x, negative operation for a boolean array is permitted (but raises DeprecationWarning).

https://github.com/numpy/numpy/blob/maintenance/1.9.x/numpy/core/src/umath/ufunc_type_resolution.c#L386-L389

It was changed to TypeError in 1.13.0:

https://github.com/numpy/numpy/blob/maintenance/1.13.x/numpy/core/src/umath/ufunc_type_resolution.c#L384-L387

Maybe we need a handling policy when NumPy deprecates / removes interfaces.

Basically we should support union of feature sets of each supported NumPy versions.
For features that are expected to behave as the same way as NumPy, test cases must be separately written for each NumPy versions.

Commit message: fix import

Patch to review:
diff --git a/cupy/core/_kernel.pyx b/cupy/core/_kernel.pyx
index 4ce74a4dadc..0fa6f2f3006 100644
--- a/cupy/core/_kernel.pyx
+++ b/cupy/core/_kernel.pyx
@@ -1115,16 +1115,18 @@ cdef class _Ops:
     cdef _Ops from_tuples(object ops, routine):
         ops_ = []
         for t in ops:
-            if isinstance(t, _Op):
-                ops_.append(t)
-            elif isinstance(t, tuple):
+            if isinstance(t, tuple):
                 typ, rt = t
                 if isinstance(rt, tuple):
                     rt = tuple([r1 or r2 for r1, r2 in zip(rt, routine)])
-                ops_.append(_Op.from_type_and_routine(typ, rt))
+                elif not isinstance(rt, str):
+                    # TODO(kataoka): assert isinstance(rt, callable)
+                    ops_.append(_Op.from_type_and_error_func(typ, rt))
+                    continue
             else:
                 assert isinstance(t, str)
-                ops_.append(_Op.from_type_and_routine(t, routine))
+                typ, rt = t, routine
+            ops_.append(_Op.from_type_and_routine(typ, rt))
         return _Ops(tuple(ops_))
 
     cdef _Op guess_routine(
diff --git a/cupy/core/_routines_math.pyx b/cupy/core/_routines_math.pyx
index eaa975b2125..7ef87dac416 100644
--- a/cupy/core/_routines_math.pyx
+++ b/cupy/core/_routines_math.pyx
@@ -363,15 +363,11 @@ cdef create_arithmetic(name, op, boolop, doc):
     #  - str (the operator for bool-bool inputs) or
     #  - callable (a function to raise an error for bool-bool inputs).
     if isinstance(boolop, str):
-        boolop_ = _kernel._Op.from_type_and_routine(
-            '??->?', 'out0 = in0 %s in1' % boolop)
-    else:
-        assert callable(boolop)
-        boolop_ = _kernel._Op.from_type_and_error_func('??->?', boolop)
+        boolop = 'out0 = in0 %s in1' % boolop
 
     return create_ufunc(
         'cupy_' + name,
-        (boolop_,
+        (('??->?', boolop),
          'bb->b', 'BB->B', 'hh->h', 'HH->H', 'ii->i', 'II->I', 'll->l',
          'LL->L', 'qq->q', 'QQ->Q', 'ee->e', 'ff->f', 'dd->d', 'FF->F',
          'DD->D'),
@@ -458,9 +454,15 @@ _imag_setter = create_ufunc(
     ''')
 
 
+def _negative_boolean_error():
+    raise TypeError(
+        'The cupy boolean negative, the `-` operator, is not supported, '
+        'use the `~` operator or the logical_not function instead.')
+
+
 _negative = create_ufunc(
     'cupy_negative',
-    (('?->?', 'out0 = !in0'),
+    (('?->?', _negative_boolean_error),
      'b->b', 'B->B', 'h->h', 'H->H', 'i->i', 'I->I', 'l->l', 'L->L',
      'q->q', 'Q->Q', 'e->e', 'f->f', 'd->d', 'F->F', 'D->D'),
     'out0 = -in0',
diff --git a/tests/cupy_tests/core_tests/test_ndarray_unary_op.py b/tests/cupy_tests/core_tests/test_ndarray_unary_op.py
index a54d03e88e6..7b697c407fb 100644
--- a/tests/cupy_tests/core_tests/test_ndarray_unary_op.py
+++ b/tests/cupy_tests/core_tests/test_ndarray_unary_op.py
@@ -2,6 +2,7 @@
 import unittest
 
 import numpy
+import pytest
 
 import cupy
 from cupy import testing
@@ -118,3 +119,20 @@ def check_zerodim_op(self, op, xp, dtype):
 
     def test_invert_zerodim(self):
         self.check_zerodim_op(operator.invert)
+
+
+@testing.parameterize(*testing.product({
+    'xp': [numpy, cupy],
+    'shape': [(3, 2), (), (3, 0, 2)]
+}))
+@testing.gpu
+class TestBoolNeg(unittest.TestCase):
+
+    def test_bool_neg(self):
+        xp = self.xp
+        if xp is numpy and not testing.numpy_satisfies('>=1.13.0'):
+            raise unittest.SkipTest('NumPy<1.13.0')
+        shape = self.shape
+        x = testing.shaped_random(shape, xp, dtype=numpy.bool_)
+        with pytest.raises(TypeError):
+            -x
diff --git a/tests/cupy_tests/math_tests/test_arithmetic.py b/tests/cupy_tests/math_tests/test_arithmetic.py
index b3e24164d18..e06892359e0 100644
--- a/tests/cupy_tests/math_tests/test_arithmetic.py
+++ b/tests/cupy_tests/math_tests/test_arithmetic.py
@@ -301,8 +301,8 @@ class TestBoolSubtract(unittest.TestCase):
 
     def test_bool_subtract(self):
         xp = self.xp
-        if xp is numpy and not testing.numpy_satisfies('>=1.13.0'):
-            raise unittest.SkipTest('NumPy<1.13.0')
+        if xp is numpy and not testing.numpy_satisfies('>=1.14.0'):
+            raise unittest.SkipTest('NumPy<1.14.0')
         shape = self.shape
         x = testing.shaped_random(shape, xp, dtype=numpy.bool_)
         y = testing.shaped_random(shape, xp, dtype=numpy.bool_)


Code review comments:
diff_hunk: @@ -1115,16 +1115,18 @@ cdef class _Ops:
     cdef _Ops from_tuples(object ops, routine):
         ops_ = []
         for t in ops:
-            if isinstance(t, _Op):
-                ops_.append(t)
-            elif isinstance(t, tuple):
+            if isinstance(t, tuple):
                 typ, rt = t
                 if isinstance(rt, tuple):
                     rt = tuple([r1 or r2 for r1, r2 in zip(rt, routine)])
-                ops_.append(_Op.from_type_and_routine(typ, rt))
+                elif not isinstance(rt, str):
+                    # TODO(kataoka): assert isinstance(rt, callable)
path: cupy/core/_kernel.pyx
text: @user1:
Why is this TODO? (It should be `assert callable(rt)` actually)

@author:
~It seems `assert callable(rt)` cannot be done in `cdef`.~

@user1:
Do you mean `assert` or `callable`, or their combination?
`assert` is used in the code already and I personally confirmed `callable` is usable in Cython.

@user1:
`assert callable(obj)` seems to work, too.

@author:
Sorry.  `assert callable(obj)` works.