Repository: numba/numba
Pull request: Fix issue 8370
Fixes #8370 

New unit tests were created, and the solution added, as suggested by @gmarkall 

All unit tests pass locally that would exercise ravel() and flatten()

- python -m numba.runtests -m -v numba.tests.test_array_reductions.TestArrayReductions
- python -m numba.runtests -m -v numba.tests.test_array_manipulation.TestArrayManipulation
- python -m numba.runtests -m -v numba.tests.test_closure.TestObjmodeFallback
- python -m numba.runtests -m -v numba.tests.test_dyn_array.TestNpStack
- python -m numba.runtests -m -v numba.tests.test_np_functions.TestNPFunctions.test_extract_basic
- python -m numba.runtests -m -v numba.tests.test_support.TestAssertPreciseEqual

CUDA tests may need to be run, but I simply cannot download the CUDA drivers from Nvidia today for some reason, so maybe a CI test would be more appropriate.



<!--

Thanks for wanting to contribute to Numba :)

First, if you need some help or want to chat to the core developers, please
visit https://gitter.im/numba/numba for real time chat or post to the Numba
forum https://numba.discourse.group/.

Here's some guidelines to help the review process go smoothly.

0. Please write a description in this text box of the changes that are being
   made.

1. Please ensure that you have written units tests for the changes made/features
   added.

2. If you are closing an issue please use one of the automatic closing words as
   noted here: https://help.github.com/articles/closing-issues-using-keywords/

3. If your pull request is not ready for review but you want to make use of the
   continuous integration testing facilities here, please click the arrow besides
   "Create Pull Request" and choose "Create Draft Pull Request".
   When it's ready for review, you can click the button "ready to review" near
   the end of the pull request
   (besides "This pull request is still a work in progress".)
   The maintainers will then be automatically notified to review it.

4. Once review has taken place please do not add features or make changes out of
   the scope of those requested by the reviewer (doing this just add delays as
   already reviewed code ends up having to be re-reviewed/it is hard to tell
   what is new etc!). Further, please do not rebase your branch on main/force
   push/rewrite history, doing any of these causes the context of any comments
   made by reviewers to be lost. If conflicts occur against main they should
   be resolved by merging main into the branch used for making the pull
   request.

Many thanks in advance for your cooperation!

-->


Problem statement:
NumbaTypeError: Cannot modify readonly array of type: readonly array(int64, 2d, C)
```
import numpy as np
from numba import njit

@njit
def unfold(x, y):
    r, c = x.shape
    a = np.broadcast_to(x, (y, r, c))
#     a = a.copy()
    b = np.swapaxes(a, 0, 1)
#     b = b.copy()
    cc = b.flatten()
#     c = c.copy()
    d = np.reshape(cc, (-1, c))
    # assign indicator to the right end of the period
    # in case of look forward bias
#     d = d.copy()
    d[y - 1 :, :] = d[: 1 - y]
    return d


unfold(np.array([[1, 1, 1], [2, 2, 2], [3,3,3]]), 2)
```
This will reproduce the error above. Under plain numpy this works fine.
By uncommenting any one of the copy() calls, the issue is resolved. So I have no idea which numpy call returns this readonly array.
Can it be implemented without an explicit copy call?

Hints from issue discussion:
Thanks for the issue report and nice reproducer! I can reproduce the issue.

The first `readonly array` is `a`, so copying any of the subsequent values breaks the "chain" of readonly arrays.
Looks like `array.flatten` erroneously types its result as `readonly` if its input is `readonly`, which is unnecessary as it returns a copy. With:

```diff
diff --git a/numba/core/typing/arraydecl.py b/numba/core/typing/arraydecl.py
index 629263aad..edb4ca103 100644
--- a/numba/core/typing/arraydecl.py
+++ b/numba/core/typing/arraydecl.py
@@ -481,7 +481,7 @@ class ArrayAttribute(AttributeTemplate):
         # Only support no argument version (default order='C')
         assert not kws
         assert not args
-        return signature(ary.copy(ndim=1, layout='C'))
+        return signature(ary.copy(ndim=1, layout='C', readonly=False))
 
     @bound_function("array.take")
     def resolve_take(self, ary, args, kws):
```

the following:

```python
import numpy as np
from numba import njit


def unfold(x, y):
    r, c = x.shape
    a = np.broadcast_to(x, (y, r, c))
#     a = a.copy()
    b = np.swapaxes(a, 0, 1)
#     b = b.copy()
    cc = b.flatten()
#     c = c.copy()
    d = np.reshape(cc, (-1, c))
    # assign indicator to the right end of the period
    # in case of look forward bias
#     d = d.copy()
    d[y - 1:, :] = d[: 1 - y]
    return d


x = np.array([[1, 1, 1], [2, 2, 2], [3, 3, 3]])

jitted = njit(unfold)

print(unfold(x, 2))
print(jitted(x, 2))
```

gives

```
$ python repro.py 
[[1 1 1]
 [1 1 1]
 [1 1 1]
 [2 2 2]
 [2 2 2]
 [3 3 3]]
[[1 1 1]
 [1 1 1]
 [1 1 1]
 [2 2 2]
 [2 2 2]
 [3 3 3]]
```

as one would expect.

To fix this, the above change can be seen as a starter patch that needs test cases (probably including the reproducer from this report) adding. This may be a good first issue for a new contributor to take on, if a maintainer doesn't get round to this first, so I'll label it as such,
So you mean broadcast_to producing a readonly array is the expected behaviour, but under plain numpy even replacing `flatten` to `ravel` works which in the numpy document says `ravel` does not necessarily copy the array. So there could be some other unconsistency in behaviour of these calls between numba and numpy?
> So you mean broadcast_to producing a readonly array is the expected behaviour

My current understanding is that this is a bug, and it shouldn't be readonly.

>  but under plain numpy even replacing `flatten` to `ravel` works which in the numpy document says `ravel` does not necessarily copy the array. So there could be some other unconsistency in behaviour of these calls between numba and numpy?

I'm not quite sure what you're suggesting here - if it seems like there's another NumPy-Numba inconsistency in `ravel`, could you please open a new issue that reproduces the inconsistency?
Well, I thought you were suggesting `flatten` is the cause. My fault, your last post make this clear. I didn't dig into numpy's source code and just making a rough judge from its document. Will see what I can do on this when I have time.
Sorry, I may be being confusing - I do think `flatten` is the cause, but the error in `flatten` is in the Numba implementation of it - I don't suspect anything wrong with NumPy itself.
(For clarity, I also don't think there's anything wrong with your code - that looks like it should work)
Since you think `flatten` is the cause, then `ravel` is not expected to work. But in numpy `flatten` and `ravel` all work fine. And in your PR, fixing `flatten` alone seems to have an impact on `ravel` as well. I'm not familiar with the compiler side of numba and a little bit confused now.

Commit message: added clarification about why we always remove the readonly flag when running a flatten()

Patch to review:
diff --git a/numba/core/typing/arraydecl.py b/numba/core/typing/arraydecl.py
index 629263aad90..e4086184f4e 100644
--- a/numba/core/typing/arraydecl.py
+++ b/numba/core/typing/arraydecl.py
@@ -474,14 +474,19 @@ def resolve_ravel(self, ary, args, kws):
         # Only support no argument version (default order='C')
         assert not kws
         assert not args
-        return signature(ary.copy(ndim=1, layout='C'))
+        copy_will_be_made = ary.layout != 'C'
+        readonly = not (copy_will_be_made or ary.mutable)
+        return signature(ary.copy(ndim=1, layout='C', readonly=readonly))
 
     @bound_function("array.flatten")
     def resolve_flatten(self, ary, args, kws):
         # Only support no argument version (default order='C')
         assert not kws
         assert not args
-        return signature(ary.copy(ndim=1, layout='C'))
+        # Just like in Numpy: "np.copy clears previously
+        # set WRITEABLE=False flag"
+        # Numba also sets readonly=False (b/c flatten returns a copy())
+        return signature(ary.copy(ndim=1, layout='C', readonly=False))
 
     @bound_function("array.take")
     def resolve_take(self, ary, args, kws):
diff --git a/numba/tests/test_array_manipulation.py b/numba/tests/test_array_manipulation.py
index d3e57893628..5731ccf126e 100644
--- a/numba/tests/test_array_manipulation.py
+++ b/numba/tests/test_array_manipulation.py
@@ -1250,6 +1250,69 @@ def test_broadcast_arrays_incompatible_shapes_raise_valueerror(self):
                 self.assertIn("shape mismatch: objects cannot be broadcast to a single shape",
                               str(raises.exception))
 
+    def test_readonly_after_flatten(self):
+        # Reproduces Issue #8370
+        def unfold_flatten(x, y):
+            r, c = x.shape
+            a = np.broadcast_to(x, (y, r, c))
+            b = np.swapaxes(a, 0, 1)
+            cc = b.flatten()
+            d = np.reshape(cc, (-1, c))
+            d[y - 1:, :] = d[: 1 - y]
+            return d
+
+        pyfunc = unfold_flatten
+        cfunc = jit(nopython=True)(pyfunc)
+
+        # If issue #8370 is not fixed: This will fail with "NumbaTypeError:
+        # Cannot modify readonly array of type: readonly array(int32, 2d, C)"
+        cfunc(np.array([[1, 1, 1], [2, 2, 2], [3, 3, 3]]), 2)
+
+
+    def test_readonly_after_ravel(self):
+        # Reproduces another suggested problem in Issue #8370
+        def unfold_ravel(x, y):
+            r, c = x.shape
+            a = np.broadcast_to(x, (y, r, c))
+            b = np.swapaxes(a, 0, 1)
+            cc = b.ravel()
+            d = np.reshape(cc, (-1, c))
+            d[y - 1:, :] = d[: 1 - y]
+            return d
+
+        pyfunc = unfold_ravel
+        cfunc = jit(nopython=True)(pyfunc)
+
+        # If issue #8370 is not fixed: This will fail with "NumbaTypeError:
+        # Cannot modify readonly array of type: readonly array(int32, 2d, C)"
+        cfunc(np.array([[1, 1, 1], [2, 2, 2], [3, 3, 3]]), 2)
+
+    def test_mutability_after_ravel(self):
+        # Reproduces another suggested problem in Issue #8370
+        # Namely that ravel should only return a writable array
+        # if a copy took place... otherwise leave it as it is.
+        self.disable_leak_check()
+        a_c = np.arange(9).reshape((3, 3)).copy()
+        a_f = a_c.copy(order='F')
+        a_c.flags.writeable = False
+        a_f.flags.writeable = False
+
+        def try_ravel_w_copy(a):
+            result = a.ravel()
+            return result
+
+        pyfunc = try_ravel_w_copy
+        cfunc = jit(nopython=True)(pyfunc)
+
+        ret_c = cfunc(a_c)
+        ret_f = cfunc(a_f)
+
+        msg = 'No copy was performed, so the ' \
+              'resulting array must not be writeable'
+        self.assertTrue(not ret_c.flags.writeable, msg)
+
+        msg = 'A copy was performed, yet the resulting array is not modifiable'
+        self.assertTrue(ret_f.flags.writeable, msg)
 
 
 if __name__ == '__main__':


Code review comments:
diff_hunk: @@ -1250,6 +1250,69 @@ def test_broadcast_arrays_incompatible_shapes_raise_valueerror(self):
                 self.assertIn("shape mismatch: objects cannot be broadcast to a single shape",
                               str(raises.exception))
 
+    def test_readonly_after_flatten(self):
+        # Reproduces Issue #8370
+        def unfold_flatten(x, y):
+            r, c = x.shape
+            a = np.broadcast_to(x, (y, r, c))
+            b = np.swapaxes(a, 0, 1)
+            cc = b.flatten()
+            d = np.reshape(cc, (-1, c))
+            d[y - 1:, :] = d[: 1 - y]
+            return d
+
+        pyfunc = unfold_flatten
+        cfunc = jit(nopython=True)(pyfunc)
+
+        # If issue #8370 is not fixed: This will fail with "NumbaTypeError:
+        # Cannot modify readonly array of type: readonly array(int32, 2d, C)"
path: numba/tests/test_array_manipulation.py
text: @user1:
This isn't the message I get - I get:

```
No implementation of function Function(<built-in function setitem>) found for signature:
 
 >>> setitem(readonly array(int64, 2d, C), UniTuple(slice<a:b> x 2), readonly array(int64, 2d, C))
```

I think it'd be OK to just delete this comment rather than update it.

diff_hunk: @@ -1250,6 +1250,69 @@ def test_broadcast_arrays_incompatible_shapes_raise_valueerror(self):
                 self.assertIn("shape mismatch: objects cannot be broadcast to a single shape",
                               str(raises.exception))
 
+    def test_readonly_after_flatten(self):
+        # Reproduces Issue #8370
+        def unfold_flatten(x, y):
+            r, c = x.shape
+            a = np.broadcast_to(x, (y, r, c))
+            b = np.swapaxes(a, 0, 1)
+            cc = b.flatten()
+            d = np.reshape(cc, (-1, c))
+            d[y - 1:, :] = d[: 1 - y]
+            return d
+
+        pyfunc = unfold_flatten
+        cfunc = jit(nopython=True)(pyfunc)
+
+        # If issue #8370 is not fixed: This will fail with "NumbaTypeError:
+        # Cannot modify readonly array of type: readonly array(int32, 2d, C)"
+        cfunc(np.array([[1, 1, 1], [2, 2, 2], [3, 3, 3]]), 2)
+
+
+    def test_readonly_after_ravel(self):
+        # Reproduces another suggested problem in Issue #8370
+        def unfold_ravel(x, y):
+            r, c = x.shape
+            a = np.broadcast_to(x, (y, r, c))
+            b = np.swapaxes(a, 0, 1)
+            cc = b.ravel()
+            d = np.reshape(cc, (-1, c))
+            d[y - 1:, :] = d[: 1 - y]
+            return d
+
+        pyfunc = unfold_ravel
+        cfunc = jit(nopython=True)(pyfunc)
+
+        # If issue #8370 is not fixed: This will fail with "NumbaTypeError:
+        # Cannot modify readonly array of type: readonly array(int32, 2d, C)"
path: numba/tests/test_array_manipulation.py
text: @user1:
Similar to above, I think this comment can / should be removed.