Repository: numpy/numpy
Pull request: BUG: Make advanced indexing result on read-only subclass writeable
Fixes #14132 

Fancy indexing on read-only subclass makes a read-only copy. This PR avoids using the flags of the original array.

Problem statement:
Fancy indexing on read-only memmap creates a read-only copy
Fancy indexing on an array creates a copy. If the array is read-only, the copy is writeable and owns its data.
```python
a = np.array([1,2,3])
a.setflags(write=False)
b = a[[1,2]]
b.flags
>>>  C_CONTIGUOUS : True
     F_CONTIGUOUS : True
     OWNDATA : True  #
     WRITEABLE : True  #
     ALIGNED : True
     WRITEBACKIFCOPY : False
     UPDATEIFCOPY : False
```
However fancy indexing on a memmap creates a read-only copy.
```python
m = np.memmap('tmp', dtype='int', mode='w+', shape=(3,))
m[:] = a[:]
m = np.memmap('tmp', dtype='int', mode='r', shape=(3,))
b = m[[1,2]]
b.flags 
>>>  C_CONTIGUOUS : True
     F_CONTIGUOUS : True
     OWNDATA : False  #
     WRITEABLE : False  #
     ALIGNED : True
     WRITEBACKIFCOPY : False
     UPDATEIFCOPY : False
```
And it seems that it does not own its data, but then who is its base ?

Hints from issue discussion:
Also, in case anyone is wondering, `b` above is an array not a mmap,
```py
>>> m
memmap([1, 2, 3])
>>> b
array([2, 3])
```
It is also contiguous even when selecting non-contiguous elements,
```py
>>> m[[0,2]].flags
  C_CONTIGUOUS : True
  F_CONTIGUOUS : True
  OWNDATA : False
  WRITEABLE : False
  ALIGNED : True
  WRITEBACKIFCOPY : False
  UPDATEIFCOPY : False
```
but still doesn't own the data.

The problem is here:
https://github.com/numpy/numpy/blob/master/numpy/core/src/multiarray/mapping.c#L1695

The writeable flag has to be cleared. The only tricky thing is to double check what flags are necessary at all (probably none, and just passing in 0 is fine).

Want to have a look?
@seberg 

I've looked at it and there's something I don't understand. When using fancy indexing, a new result array is created:
```c
result = PyArray_NewFromDescr(&PyArray_Type,
                              PyArray_DESCR(self),
                              PyArray_NDIM(ind),
                              PyArray_SHAPE(ind),
                              NULL, NULL,
                              /* Same order as indices */
                              PyArray_ISFORTRAN(ind) ?
                                  NPY_ARRAY_F_CONTIGUOUS : 0,
                              NULL);
```

and then, a new one may be created with this one as base:
```c
wrap_out_array:
    if (!PyArray_CheckExact(self)) {
        /*
         * Need to create a new array as if the old one never existed.
         */
        PyArrayObject *tmp_arr = (PyArrayObject *)result;

        Py_INCREF(PyArray_DESCR(tmp_arr));
        result = PyArray_NewFromDescrAndBase(
                Py_TYPE(self),
                PyArray_DESCR(tmp_arr),
                PyArray_NDIM(tmp_arr),
                PyArray_SHAPE(tmp_arr),
                PyArray_STRIDES(tmp_arr),
                PyArray_BYTES(tmp_arr),
                PyArray_FLAGS(self),
                (PyObject *)self, (PyObject *)tmp_arr);
        Py_DECREF(tmp_arr);
        if (result == NULL) {
            goto finish;
        }
    }
```

Why is this step necessary ? What is the purpose of PyArray_CheckExact ?
In which situation returning the previously created array is problematic ?
@jeremiedbb the second call is for subclasses. The first call always returns an "exact" (non-subclass) numpy array. Since the first call only has the contiguity as flags, I do not think the second one requires any flags at all.
The second call uses the flags of `self`. Using the flags of `tmp_arr` solves the writeable issue.

However it does not solve the owndata issue. Since it's a call of `PyArray_NewFromDescrAndBase`, its base is `tmp_arr`. But it should not have a base, right ?

Why returning the array after the first call is not enough ?
The owndata is part is not an issue, there is nothing wrong with it having a base. This is because the subclass may need the data.

Commit message: do not use original array flags in fancy indexing

Patch to review:
diff --git a/numpy/core/src/multiarray/mapping.c b/numpy/core/src/multiarray/mapping.c
index add1143b2d63..9bb85e320b05 100644
--- a/numpy/core/src/multiarray/mapping.c
+++ b/numpy/core/src/multiarray/mapping.c
@@ -1699,7 +1699,7 @@ array_subscript(PyArrayObject *self, PyObject *op)
                 PyArray_SHAPE(tmp_arr),
                 PyArray_STRIDES(tmp_arr),
                 PyArray_BYTES(tmp_arr),
-                PyArray_FLAGS(self),
+                PyArray_FLAGS(tmp_arr),
                 (PyObject *)self, (PyObject *)tmp_arr);
         Py_DECREF(tmp_arr);
         if (result == NULL) {
diff --git a/numpy/core/tests/test_indexing.py b/numpy/core/tests/test_indexing.py
index f7485c3f7c9e..31492a39f16b 100644
--- a/numpy/core/tests/test_indexing.py
+++ b/numpy/core/tests/test_indexing.py
@@ -617,6 +617,19 @@ class SubClass(np.ndarray):
         assert_array_equal(s_bool, a[a > 0])
         assert_array_equal(s_bool.base, a[a > 0])
 
+    def test_fancy_on_read_only(self):
+        # Test that fancy indexing on read-only SubClass does not make a
+        # read-only copy (gh-14132)
+        class SubClass(np.ndarray):
+            pass
+
+        a = np.arange(5)
+        s = a.view(SubClass)
+        s.setflags(write=False)
+        s_fancy = s[[0, 1, 2]]
+        assert_(s_fancy.flags['WRITEABLE'])
+
+
     def test_finalize_gets_full_info(self):
         # Array finalize should be called on the filled array.
         class SubClass(np.ndarray):


Code review comments:
diff_hunk: @@ -617,6 +617,19 @@ class SubClass(np.ndarray):
         assert_array_equal(s_bool, a[a > 0])
         assert_array_equal(s_bool.base, a[a > 0])
 
+    def test_fancy_on_read_only(self):
+        # Test that fancy indexing on read-only SubClass does not make a
+        # read-only copy (gh-14132)
+        class SubClass(np.ndarray):
+            pass
+
+        a = np.arange(5)
+        s = a.view(SubClass)
+        s.setflags(write=False)
path: numpy/core/tests/test_indexing.py
text: @user1:
```suggestion
        s.flags.writeable = False
```

diff_hunk: @@ -617,6 +617,19 @@ class SubClass(np.ndarray):
         assert_array_equal(s_bool, a[a > 0])
         assert_array_equal(s_bool.base, a[a > 0])
 
+    def test_fancy_on_read_only(self):
+        # Test that fancy indexing on read-only SubClass does not make a
+        # read-only copy (gh-14132)
+        class SubClass(np.ndarray):
+            pass
+
+        a = np.arange(5)
+        s = a.view(SubClass)
+        s.setflags(write=False)
+        s_fancy = s[[0, 1, 2]]
+        assert_(s_fancy.flags['WRITEABLE'])
path: numpy/core/tests/test_indexing.py
text: @user1:
Nit: would be good to spell the flag the same way for both setting and getting.
```suggestion
        assert_(s_fancy.flags.writeable)
```