Repository: numba/numba
Pull request: Inconsistent behavior of reshape between numpy and numba/cuda device array
 <!--

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 please label it with `[WIP]` and
   then remove the label when you'd like it to be reviewed.

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 master/force
   push/rewrite history, doing any of these causes the context of any comments
   made by reviewers to be lost. If conflicts occur against master they should
   be resolved by merging master into the branch used for making the pull
   request.

Many thanks in advance for your cooperation!

-->
Fixes #6930. 

A branch is added to check for a single `-1` instance, or an unknown dimension, in the `newdims`. If there is an unknown dimension, the computation is done to work out the missing dimension. Finally, `newdims` is updated with `-1` replaced with the computed dimension at the appropriate index to reflect this.

Unknown dimension inference based off of [numpy's implementation](https://github.com/numpy/numpy/blob/623bc1fae1d47df24e7f1e29321d0c0ba2771ce0/numpy/core/src/multiarray/shape.c#L470-L515).

Problem statement:
Inconsistent behavior of reshape between numpy and numba/cuda device array
Consider the below code. Numpy will happily reshape using -1 to fill in any missing dimension, numba/cuda will not. Please mirror numpy behavior.
```
import numpy as np
import numba as nb
from numba import cuda
sixty_four = np.zeros(64)

blocks_of_thirty_two = sixty_four.reshape(-1,32)
nb_sixty_four = cuda.to_device(sixty_four)
nb_sixty_four.reshape(-1, 32)
```

Hints from issue discussion:
Thanks for the report, I can reproduce. This seems to simply be not implemented but should be relatively easy to do. A branch needs adding here (referenced below) to check for a single `-1` instance in the `newdims` and then the computation doing to work out the missing dimension and then an update to `newdims` to reflect this.
https://github.com/numba/numba/blob/12355c16ef70d10b0812c4266a3e143bd135a7f1/numba/misc/dummyarray.py#L316-L323
I'd love to contribute to Numba and take this on as a good first issue! Found the numpy implementation of computing the missing dimension [here](https://github.com/numpy/numpy/blob/623bc1fae1d47df24e7f1e29321d0c0ba2771ce0/numpy/core/src/multiarray/shape.c#L470-L515) and am replicating similar logic in the section of `dummyarray.py` referenced by @stuartarchibald.  
> I'd love to contribute to Numba and take this on as a good first issue! Found the numpy implementation of computing the missing dimension [here](https://github.com/numpy/numpy/blob/623bc1fae1d47df24e7f1e29321d0c0ba2771ce0/numpy/core/src/multiarray/shape.c#L470-L515) and am replicating similar logic in the section of `dummyarray.py` referenced by @stuartarchibald.

Great, thanks @laurenarnett !

As a new contributor, some instructions for getting set up for development are [here](http://numba.pydata.org/numba-doc/latest/developer/contributing.html#getting-set-up) and are a great place to start.

Commit message: add reshape test for infer unknown dimension

Patch to review:
diff --git a/numba/misc/dummyarray.py b/numba/misc/dummyarray.py
index 07c5c16be06..bdaa9973794 100644
--- a/numba/misc/dummyarray.py
+++ b/numba/misc/dummyarray.py
@@ -314,6 +314,27 @@ def reshape(self, *newdims, **kws):
         if order not in 'CFA':
             raise ValueError('order not C|F|A')
 
+        # check for exactly one instance of -1 in newdims
+        unknownidx = -1
+        knownsize = 1
+        for i, dim in enumerate(newdims):
+            if dim < 0:
+                if unknownidx == -1:
+                    unknownidx = i
+                else:
+                    raise ValueError("can only specify one unknown dimension")
+            else:
+                knownsize *= dim
+
+        # compute the missing dimension
+        if unknownidx >= 0:
+            if knownsize == 0 or self.size % knownsize != 0:
+                raise ValueError("cannot infer valid shape for unknown dimension")
+            else:
+                newdims = newdims[0:unknownidx] \
+                        + tuple((self.size // knownsize,)) \
+                        + newdims[unknownidx + 1:]
+
         newsize = functools.reduce(operator.mul, newdims, 1)
 
         if order == 'A':
diff --git a/numba/tests/test_dummyarray.py b/numba/tests/test_dummyarray.py
index beb4e8b10ba..1be4aa03a0f 100644
--- a/numba/tests/test_dummyarray.py
+++ b/numba/tests/test_dummyarray.py
@@ -208,6 +208,51 @@ def test_reshape_3d1d(self):
         self.assertEqual(got.shape, expect.shape)
         self.assertEqual(got.strides, expect.strides)
 
+    def test_reshape_infer2d2d(self):
+        nparr = np.empty((4, 5))
+        arr = Array.from_desc(0, nparr.shape, nparr.strides,
+                              nparr.dtype.itemsize)
+        expect = nparr.reshape(5, 4)
+        got = arr.reshape(-1, 4)[0]
+        self.assertEqual(got.shape, expect.shape)
+        self.assertEqual(got.strides, expect.strides)
+
+    def test_reshape_infer2d1d(self):
+        nparr = np.empty((4, 5))
+        arr = Array.from_desc(0, nparr.shape, nparr.strides,
+                              nparr.dtype.itemsize)
+        expect = nparr.reshape(5 * 4)
+        got = arr.reshape(-1)[0]
+        self.assertEqual(got.shape, expect.shape)
+        self.assertEqual(got.strides, expect.strides)
+
+    def test_reshape_infer3d3d(self):
+        nparr = np.empty((3, 4, 5))
+        arr = Array.from_desc(0, nparr.shape, nparr.strides,
+                              nparr.dtype.itemsize)
+        expect = nparr.reshape(5, 3, 4)
+        got = arr.reshape(5, -1, 4)[0]
+        self.assertEqual(got.shape, expect.shape)
+        self.assertEqual(got.strides, expect.strides)
+
+    def test_reshape_infer3d2d(self):
+        nparr = np.empty((3, 4, 5))
+        arr = Array.from_desc(0, nparr.shape, nparr.strides,
+                              nparr.dtype.itemsize)
+        expect = nparr.reshape(3, 4 * 5)
+        got = arr.reshape(3, -1)[0]
+        self.assertEqual(got.shape, expect.shape)
+        self.assertEqual(got.strides, expect.strides)
+
+    def test_reshape_infer3d1d(self):
+        nparr = np.empty((3, 4, 5))
+        arr = Array.from_desc(0, nparr.shape, nparr.strides,
+                              nparr.dtype.itemsize)
+        expect = nparr.reshape(3 * 4 * 5)
+        got = arr.reshape(-1)[0]
+        self.assertEqual(got.shape, expect.shape)
+        self.assertEqual(got.strides, expect.strides)
+
 
 class TestSqueeze(unittest.TestCase):
     def test_squeeze(self):


Code review comments:
diff_hunk: @@ -314,6 +314,27 @@ def reshape(self, *newdims, **kws):
         if order not in 'CFA':
             raise ValueError('order not C|F|A')
 
+        # check for exactly one instance of -1 in newdims
+        unknownidx = -1
+        knownsize = 1
+        for i, dim in enumerate(newdims):
+            if dim < 0:
+                if unknownidx == -1:
+                    unknownidx = i
+                else:
+                    raise ValueError("can only specify one unknown dimension")
+            else:
+                knownsize *= dim
+
+        # compute the missing dimension
+        if unknownidx >= 0:
+            if knownsize == 0 or self.size % knownsize != 0:
+                raise ValueError("cannot infer valid shape for unknown dimension")
+            else:
+                newdims = newdims[0:unknownidx] \
+                        + tuple((self.size // knownsize,)) \
path: numba/misc/dummyarray.py
text: @user1:
I think the `tuple()` call is unnecessary here as `(self.size // knownsize,)` already creates a tuple.
```suggestion
                        + (self.size // knownsize,) \
```

diff_hunk: @@ -208,6 +208,51 @@ def test_reshape_3d1d(self):
         self.assertEqual(got.shape, expect.shape)
         self.assertEqual(got.strides, expect.strides)
 
+    def test_reshape_infer2d2d(self):
+        nparr = np.empty((4, 5))
+        arr = Array.from_desc(0, nparr.shape, nparr.strides,
+                              nparr.dtype.itemsize)
+        expect = nparr.reshape(5, 4)
path: numba/tests/test_dummyarray.py
text: @user1:
Although this is equal to the shape we'd expect, it does feel a little like we're not quite testing like-for-like here, because we're not creating the expected array with a missing dimension. Perhaps:
```suggestion
        expect = nparr.reshape(-1, 4)
```
would be more explicit that we're testing like-for-like?

(similar applies to the subsequent four `nparr.reshape` calls below too).

@author:
I had been thinking that by explicitly putting the result of the inferred dimension in the expected, it would make it clear that we're ensuring that the logic for working out the missing dimension is working appropriately. But I'm realizing now it's sufficient to match the numpy behavior. I agree that is more explicit in testing like-for-like!