Repository: cupy/cupy
Pull request: Fix condition to use slice copy in ndarray.__setitem__
Fix #3013 .

This PR fixes the condition to use CUPY_EXPERIMENTAL_SLICE_COPY feature in `ndarray.__setitem__` to check if the supplied `slices` is a slice object.

Problem statement:
[Bug] Mask assignment throws an error when CUPY_EXPERIMENTAL_SLICE_COPY=1
An error occurs when setting CUPY_EXPERIMENTAL_SLICE_COPY = 1

  - CuPy version: 6.6.0
  - OS/Platform: Ubuntu 16.04.6 LTS
  - CUDA version: 10.0

* Code to reproduce
```
import os
os.environ["CUPY_EXPERIMENTAL_SLICE_COPY"] = "1"
import cupy as cp

a = cp.array([1,2,3,4,5])
a[a<3] = 0
a
```
* Error messages, stack traces, or logs
```
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-1-9ff589a008e4> in <module>
      4 
      5 a = cp.array([1,2,3,4,5])
----> 6 a[a<3] = 0
      7 a

cupy/core/core.pyx in cupy.core.core.ndarray.__setitem__()

cupy/core/core.pyx in cupy.core.core.ndarray.__richcmp__()

cupy/core/_kernel.pyx in cupy.core._kernel.ufunc.__call__()

cupy/core/_kernel.pyx in cupy.core._kernel._preprocess_args()

TypeError: Unsupported type <class 'slice'>
```

Hints from issue discussion:
@pentschev Thanks!
Thanks for the reporting. I reproduced the issue, let me check its detail.
When CUPY_EXMERIMENTAL_SLICE_COPY is enabled, a comparison ufunc is invoked between a ndarray and a slicing object in the following line, causing the type error above:

https://github.com/cupy/cupy/blob/9da978ba0b4fa0605d4755fa00968a7fefe8075f/cupy/core/core.pyx#L1239-L1240

Commit message: Flake8

Patch to review:
diff --git a/cupy/core/core.pyx b/cupy/core/core.pyx
index 5d2a88b1cf4..7eb0f4ee9e3 100644
--- a/cupy/core/core.pyx
+++ b/cupy/core/core.pyx
@@ -1233,10 +1233,13 @@ cdef class ndarray:
             array([9998., 9999.])
 
         """
-        if (util.ENABLE_SLICE_COPY and slices == slice(None, None, None) and
-                isinstance(value, numpy.ndarray)):
-            if (self.dtype == value.dtype and
-                    self.shape == value.shape):
+        def does_slice_copy(slices, value):
+            return (type(slices) is slice and
+                    slices == slice(None, None, None) and
+                    isinstance(value, numpy.ndarray))
+
+        if util.ENABLE_SLICE_COPY and does_slice_copy(slices, value):
+            if self.dtype == value.dtype and self.shape == value.shape:
                 if self.strides == value.strides:
                     ptr = ctypes.c_void_p(value.__array_interface__['data'][0])
                 else:
diff --git a/tests/cupy_tests/core_tests/test_ndarray_copy_and_view.py b/tests/cupy_tests/core_tests/test_ndarray_copy_and_view.py
index c4a83a9b0be..c0fbce23cd2 100644
--- a/tests/cupy_tests/core_tests/test_ndarray_copy_and_view.py
+++ b/tests/cupy_tests/core_tests/test_ndarray_copy_and_view.py
@@ -193,6 +193,13 @@ def test_isinstance_numpy_copy_wrong_shape(self, xp):
         b = cupy.empty(100, dtype=a.dtype)
         b[:] = a
 
+    @unittest.skipUnless(util.ENABLE_SLICE_COPY, 'Special copy disabled')
+    @testing.numpy_cupy_array_equal()
+    def test_isinstance_numpy_copy_not_slice(self, xp):
+        a = xp.arange(5, dtype=numpy.float64)
+        a[a < 3] = 0
+        return a
+
 
 @testing.parameterize(
     {'src_order': 'C'},


Code review comments:
diff_hunk: @@ -1233,11 +1233,13 @@ cdef class ndarray:
             array([9998., 9999.])
 
         """
-        if (util.ENABLE_SLICE_COPY and
-            type(slices) is slice and slices == slice(None, None, None) and
-            isinstance(value, numpy.ndarray)):
-            if (self.dtype == value.dtype and
-                    self.shape == value.shape):
+        def does_slice_copy(slices, value):
+            return (type(slices) is slice and
+                    slices == slice(None, None, None) and
+                    isinstance(value, numpy.ndarray))
+
+        if util.ENABLE_SLICE_COPY and does_slice_copy(slices, value):
path: cupy/core/core.pyx
text: @user1:
- I'm not sure the overhead of the function call is negligible.
- (nit) Break lines before operators.
```suggestion
        if util.ENABLE_SLICE_COPY and (
                type(slices) is slice
                and slices == slice(None, None, None)
                and isinstance(value, numpy.ndarray)
        ):
```

@author:
Sure!