Repository navigation
BUG: Fix uint-overflow if padding with linear_ramp and negative gain - #14209
Conversation
charris
added
00 - Bug
06 - Regression
09 - Backport-Candidate
|
CI failures seem unrelated. |
|
I'm not sure how the "09 - Backport-Candidate" label is understood here but simply cherry-picking these changes won't work on the 1.16-release because of the rewrite of pad's internals in the meantime... 1.17 should be fine though. |
|
Oh, sorry. I thought this was a regression. It is a bug that exists seperately both on 1.16 and 1.7 I guess. |
|
@seberg ping |
|
@eric-wieser Ping. |
|
Will take a look when I'm back from vacation next week |
|
Ping all :) |
|
Pushing off to 1.17.2. Will still put in in 1.17.1 if done in time. |
|
I note that there is already a backport for 1.16.5. I think that should be closed until this is ready and merged. |
|
Well benchmarks are a little disheartening (shows only changes with a factor >= 1.1): A few cases for higher dimensions seem to be affected only. Having looked at how linspace is implemented I could try to copy/adapt the type handling into the old _linear_ramp method if you feel the optimization is worth it. |
There was a problem hiding this comment.
Does any of this docstring need transferring to below?
There was a problem hiding this comment.
In my opinion, no. All parameters of _get_linear_ramps are already documented and _linear_ramp is fully replaced by linspace. Oh, and edge_pair is documented as the return value of _get_edges. Should be fine.
There was a problem hiding this comment.
I don't think the +1 works correctly if axis is negative.
If you squeeze before linspace, I think the problem will go away - start=end_value_pair[1].squeeze(axis) etc
For that to work, you might first have to use something like
end_value_pair = [
np.broadcast_to(end_v, edge_v.shape)
for end_v, edge_v in zip(end_value_pair, edge_pair)
]
There was a problem hiding this comment.
Actually, I'm now wondering if we should add a keepdims=True to linspace, with semantics "replace the axisth axis with the new linspace and squeeze". @mhvk, do you have thoughts on this?
There was a problem hiding this comment.
@eric-wieser - you mean something like the opposite of a reduction, where keepdims=True tells linspace that there is already a dimension of 1 inside start and stop? On the one hand, it doesn't quite feel as natural, since in that case one has to check that that dimension is indeed 1. On the other hand, I can see the nice aspect that then start and stop would broadcast with the result. Overall, the "other hand" wins for me...
There was a problem hiding this comment.
If you squeeze before linspace, I think the problem will go away - start=end_value_pair[1].squeeze(axis) etc
That did the trick. I think broadcasting the end_value_pair is not necessary. linspace does that itself when constructing delta. Testing seems to confirm that.
linspace supports non-scalar values since version 1.16. This can replace the former _linear_ramp method. This removes a bug in the old implementation where certain edge and end values resulted in an integer underflow for unsigned dtypes (see numpygh-14191). Protect against this regression with a new test for all numeric dtypes.
|
@eric-wieser Ping. |
eric-wieser
left a comment
eric-wieser
left a comment
There was a problem hiding this comment.
Really pleased that the linspace improvements allowed a bunch of code to be deleted here!
|
You're welcome but the important ideas where all yours. 😉 |
In case end_value was greater than the edge values of the array to pad, the resulting difference was negative and caused overflows in case the input array used unsigned types. Fixed by using 1.17 version of
linspaceinstead of_linear_ramp.Protect against this regression with a new test for all numeric dtypes.
Fixes #14191.
cc @seberg, @aleju