BUG: `np.resize` negative shape and subclasses edge case fixes #15037

tirthasheshpatel · 2019-12-03T12:34:33Z

eric-wieser · 2019-12-03T12:48:38Z

I think you should be raising an error when the size is negative, not just forcing it to be positive

tirthasheshpatel · 2019-12-03T13:09:23Z

eric-wieser · 2019-12-03T13:09:24Z

numpy/core/tests/test_numeric.py

@@ -48,6 +48,14 @@ def test_reshape_from_zero(self):
        assert_array_equal(Ar, np.zeros((2, 1), Ar.dtype))
        assert_equal(A.dtype, Ar.dtype)

+    def test_negativeresize(self):
+        A = np.arange(0, 10, dtype=np.float32)
+        new_shape = (10,-1)


Can you add a test for (-10, -1) too, which should also fail?

(hint: It won't fail, you need to adjust your condition above)

I think this should be resolved now

eric-wieser · 2019-12-03T13:09:55Z

numpy/core/tests/test_numeric.py

+        new_shape = (10,-1)
+        err_msg = (r"new_shape can't have negative values."
+                   r" found: new_shape=(10,-1)")
+        with pytest.raises(ValueError, match=err_msg):


match="negative" would probably be enough here - matching the full message makes the test overly brittle to wording changes.

I dont know why the tests are failing with the current fix! Can you please guide me through it? Thanks for your replies!

You can see the failing test result here:

https://dev.azure.com/numpy/numpy/_build/results?buildId=6722&view=ms.vss-test-web.build-test-results-tab&runId=1078974&resultId=100000&paneView=debug

There's a bug in the error handling, opened gh-15039

Ohk! Thanks!

eric-wieser · 2019-12-03T16:14:38Z

Close / reopen to rerun tests now that #15039 is in

eric-wieser · 2019-12-03T19:26:33Z

Still failing, but with a better traceback.

Something very weird is going on here:

https://dev.azure.com/numpy/numpy/_build/results?buildId=6726&view=ms.vss-test-web.build-test-results-tab&runId=1079010&resultId=100000&paneView=debug

seberg · 2019-12-03T20:01:07Z

The reason is probably that any is actually np.any here, which misbehaves for generators. It should be using __builtin__.any. The best solution is probably to avoid importing the numpy any at all.

EDIT: Oh, any is actually defined in this file. Maybe spelling it __builtin__.any is actually fine...

seberg · 2019-12-03T20:39:56Z

I pushed a commit trying that (some hopefully slight rewording/style fixes).

numpy/core/fromnumeric.py

tirthasheshpatel · 2019-12-07T07:51:22Z

I am so sorry to add some bad commit from my work on another issue!
@seberg can you please add the previous commit again!

seberg · 2019-12-23T21:26:02Z

Sorry, I had lost track of this. Forced push the old version here. But I think we may try one from the discussion I had with Eric. Do you want to have a look, otherwise please ping me so we can maybe just fix this up.

eric-wieser · 2020-01-10T12:31:14Z

@seberg want to finish this up? Perhaps the implementation should become:

ret = np.zeros_like(a, shape=new_shape, order="C")
ret.flat[:] = a  # if a.size == 0 this is a no-op, hence the `zeros` above
return ret

seberg · 2020-01-16T19:29:44Z

So... turned out the .flat solution does not work because MaskedArray.flat is probably utterly broken. In an attempt to not change the annoying subclassing semantics, I just went back to the first solution (except avoiding any, by iterating manually because it makes no real difference anyway).

The concatenate(..., out=...) idea cannot work since the new array is smaller (plus I do not trust these things with strange subclasses for now).

There is an additional bug fix, since it should be np.zeros_like() and not np.zeros...

eric-wieser · 2020-01-16T22:16:14Z

numpy/core/fromnumeric.py

+    extra = new_size % a.size
+    repeats = (new_size - extra) // a.size


Shorter and perhaps faster as

repeats, extra = divmod(new_size, a.size)

edit: actually, I think what you have here is wrong - shouldn't repeats round up, not down? That can be spelt as:

repeats, extra = divmod(newsize,-a.size) repeats = -repeats

or perhaps more clearly

repeats = -(-newsize // a.size) # ceil division a = concatenate((a,) * repeats) a = a[:newsize]

There is no down rounding here, since the result is always an exact integer (the // only makes sure it remains an int)? But yes, divmod is better, somehow forgot it is a python function/operator...

There is rounding down, you did it via the subtraction.

As I read it, this will make np.arange(5).resize(7) give an array of the wrong size - your division will give 1, and then extra will be 2 so you'll return an array of length 3.

Sorry, yeah, I guess I would have thought such a big error gets found by the tests :/

Jeesh... emberassing. I just opted for duplicating the concatenate line to keep it stupid, open for other things, but...

Is there a test for np.arange(5).resize(7)?

There is now...

numpy/core/fromnumeric.py

eric-wieser · 2020-01-17T12:09:52Z

numpy/core/tests/test_numeric.py

+
+    def test_subclass(self):
+        class MyArray(np.ndarray):
+            __array_priority__ = 1.


What happens without this member?

concatenate seems to lose the subclass info. That could actually be a bug, since all inputs are MyArray.

Doesn't concatenate always lose subclass info?

Hmm, no, the test would fail if it did :).

numpy/core/fromnumeric.py

When a negative entry is passed into `np.resize` shape, an error will now be raised reliably. Previously the `-1` semantics of reshape allowed edge cases to pass and return incorrect results. In the corner case of an empty result shape or empty input shape, the result type will now be preserved.

seberg · 2020-05-26T20:59:55Z

I think this was ready, I am a bit hesitant to merge though, since I was the one who pushed the last changed.

eric-wieser · 2020-05-26T21:23:34Z

numpy/core/fromnumeric.py

-    extra = total_size % Na
+    new_size = 1
+    for dim_length in new_shape:
+        new_size *= dim_length


Could put this below the error check for a marginal speed boost in the error case, but doesn't really matter.

Doubt its nicer to read, and I refuse to optimize error-cases without good reason :). Thanks for another pass Eric lets put it in.

eric-wieser added the 59 - Needs tests label Dec 3, 2019

eric-wieser reviewed Dec 3, 2019

View reviewed changes

eric-wieser removed the 59 - Needs tests label Dec 3, 2019

tirthasheshpatel requested a review from eric-wieser December 3, 2019 13:38

eric-wieser mentioned this pull request Dec 3, 2019

BUG: Exceptions tracebacks are dropped #15039

Merged

eric-wieser closed this Dec 3, 2019

eric-wieser reopened this Dec 3, 2019

eric-wieser reviewed Dec 3, 2019

View reviewed changes

numpy/core/fromnumeric.py Show resolved Hide resolved

eric-wieser mentioned this pull request Jan 9, 2020

FIX: Added check for negative dimensions passed to np.resize() #15302

Closed

eric-wieser reviewed Jan 16, 2020

View reviewed changes

seberg changed the title ~~FIX: negative shape in np.resize is now handled~~ BUG: np.resize negative shape and subclasses edge case fixes Jan 16, 2020

eric-wieser reviewed Jan 17, 2020

View reviewed changes

numpy/core/fromnumeric.py Outdated Show resolved Hide resolved

eric-wieser reviewed Jan 17, 2020

View reviewed changes

numpy/core/fromnumeric.py Outdated Show resolved Hide resolved

seberg added the 58 - Ready for review label Mar 4, 2020

mattip mentioned this pull request Mar 29, 2020

BUG: numpy.resize returns wrong size when using -1 syntax for axis length. #15030

Closed

Qiyu8 added 01 - Enhancement component: numpy._core labels May 25, 2020

Qiyu8 requested review from seberg and eric-wieser May 25, 2020 08:09

seberg approved these changes May 26, 2020

View reviewed changes

eric-wieser reviewed May 26, 2020

View reviewed changes

eric-wieser approved these changes May 26, 2020

View reviewed changes

seberg merged commit 171ea2a into numpy:master May 26, 2020

eric-wieser mentioned this pull request Aug 14, 2020

np.resize crashes when the product of the array dimensions overflows an int32 #17080

Closed

tirthasheshpatel deleted the patch-1 branch October 16, 2020 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: `np.resize` negative shape and subclasses edge case fixes #15037

BUG: `np.resize` negative shape and subclasses edge case fixes #15037

tirthasheshpatel commented Dec 3, 2019

eric-wieser commented Dec 3, 2019

tirthasheshpatel commented Dec 3, 2019

eric-wieser Dec 3, 2019

tirthasheshpatel Dec 3, 2019

eric-wieser Dec 3, 2019

tirthasheshpatel Dec 3, 2019

eric-wieser Dec 3, 2019

eric-wieser Dec 3, 2019

tirthasheshpatel Dec 3, 2019

eric-wieser commented Dec 3, 2019

eric-wieser commented Dec 3, 2019

seberg commented Dec 3, 2019 •

edited

Loading

seberg commented Dec 3, 2019

tirthasheshpatel commented Dec 7, 2019

seberg commented Dec 23, 2019

eric-wieser commented Jan 10, 2020

seberg commented Jan 16, 2020

eric-wieser Jan 16, 2020 •

edited

Loading

seberg Jan 16, 2020

eric-wieser Jan 16, 2020

seberg Jan 16, 2020

seberg Jan 16, 2020

mattip Jan 16, 2020

seberg Jan 16, 2020

mattip Jan 17, 2020

eric-wieser Jan 17, 2020

seberg Jan 17, 2020

eric-wieser Jan 17, 2020

seberg Jan 17, 2020

seberg commented May 26, 2020

eric-wieser May 26, 2020

seberg May 26, 2020

		extra = new_size % a.size
		repeats = (new_size - extra) // a.size

BUG: np.resize negative shape and subclasses edge case fixes #15037

BUG: np.resize negative shape and subclasses edge case fixes #15037

Conversation

tirthasheshpatel commented Dec 3, 2019

eric-wieser commented Dec 3, 2019

tirthasheshpatel commented Dec 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser commented Dec 3, 2019

eric-wieser commented Dec 3, 2019

seberg commented Dec 3, 2019 • edited Loading

seberg commented Dec 3, 2019

tirthasheshpatel commented Dec 7, 2019

seberg commented Dec 23, 2019

eric-wieser commented Jan 10, 2020

seberg commented Jan 16, 2020

eric-wieser Jan 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seberg commented May 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BUG: `np.resize` negative shape and subclasses edge case fixes #15037

BUG: `np.resize` negative shape and subclasses edge case fixes #15037

seberg commented Dec 3, 2019 •

edited

Loading

eric-wieser Jan 16, 2020 •

edited

Loading