BUG: Fix strange behavior of infinite-step-size/underflow-case in arange #10263
Conversation
numpy/core/src/multiarray/ctors.c
Outdated
| return -1; | ||
|
|
||
| if (val_is_zero && next_is_nonzero) { | ||
| if (signbit(value)) { |
|
Thanks @xoviat, fixed! |
| delta = stop - start; | ||
| tmp_len = delta/step; | ||
|
|
||
| if (tmp_len == 0.0 && delta != 0.0) { |
There was a problem hiding this comment.
This needs a comment explaining why it is a special case
Isn't this only satisfied if step is inf anyway?
There was a problem hiding this comment.
@eric-wieser This also considers some underflow cases. eg. in master branch,
>>> m = np.float32(3.40282347E+38)
>>> e = np.float32(1.401298E-45)
>>> m
3.4028235e+38
>>> e
1.4012985e-45
>>> e/m
0.0
>>> np.arange(np.float32(0), e, m)
array([], dtype=float64)|
@eric-wieser Added comments. |
numpy/core/src/multiarray/ctors.c
Outdated
There was a problem hiding this comment.
Don't use C++ comments, use /* ... */.
96b261d to
d4d2758
Compare
| return -1; | ||
| } | ||
|
|
||
| val_is_zero = PyObject_RichCompareBool(val, zero, Py_EQ); |
There was a problem hiding this comment.
Should be an int, not an intp.
| return -1; | ||
| } | ||
|
|
||
| next_is_nonzero = PyObject_RichCompareBool(*next, zero, Py_NE); |
There was a problem hiding this comment.
Could use PyObject_IsTrue, but that might be worse on unusual types
There was a problem hiding this comment.
@eric-wieser IMO, PyObject_RichCompareBool is better because this is clear what we do.
| *next = NULL; | ||
|
|
||
| if (!val) { | ||
| return -1; |
|
Thanks @eric-wieser, fixed! |
numpy/core/src/multiarray/ctors.c
Outdated
There was a problem hiding this comment.
Need to DECREF zero here too - or better yet, just allocate it further down.
numpy/core/src/multiarray/ctors.c
Outdated
There was a problem hiding this comment.
Really you should check the return value is not NULL here too.
5f6b735 to
41cd4e1
Compare
|
@eric-wieser Fixed! |
|
Also, I need to add some underflow-case tests. |
numpy/core/src/multiarray/ctors.c
Outdated
|
|
||
| *next = PyNumber_Subtract(stop, start); | ||
| if (!(*next)) { | ||
| Py_DECREF(zero); |
There was a problem hiding this comment.
If you move the allocation of zero down below this if, then you don't need to clear it up here
There was a problem hiding this comment.
@eric-wieser If I move down zero allocation, we need to DECREF *next and set NULL to *next when the zero allocation failed. IMO, this is complicated.
There was a problem hiding this comment.
and set NULL to *next when the zero allocation failed.
You already need to do this
|
Did you mean to refer to #10271 in the PR description? Seems unrelated to me |
|
@eric-wieser I did not refer to that issue. Maybe @charris added this.
|
|
Thanks @eric-wieser, fixed! |
|
Added the test for some underflow cases |
| val_is_zero = PyObject_RichCompareBool(val, zero, Py_EQ); | ||
| Py_DECREF(zero); | ||
|
|
||
| if (cmplx && PyComplex_Check(val)) { |
There was a problem hiding this comment.
Trying to work out if this section needs a fix too, but I can't work out what this is even trying to do.
There was a problem hiding this comment.
@eric-wieser Me too. I don't know why the complex case exists. This is none-sense as math.
|
Can you extract |
There was a problem hiding this comment.
On second thoughts, this is fine as is - I'll leave it open for a few more days to let someone else double-check refcounts, and then squash and merge your commits.
numpy/core/src/multiarray/ctors.c
Outdated
| length = -1.0; | ||
| } | ||
| else { | ||
| length = 1.0; |
There was a problem hiding this comment.
nit: should be 1, as this is an integer, not double, variable. Same above.
numpy/core/src/multiarray/ctors.c
Outdated
| len = -1.0; | ||
| } | ||
| else { | ||
| len = 1.0; |
numpy/core/src/multiarray/ctors.c
Outdated
| /* Underflow and divide-by-inf check */ | if (val_is_zero && next_is_nonzero) { | |
| if (npy_signbit(value)) { | ||
| len = -1.0; |
There was a problem hiding this comment.
I think this should be 0. as it is effectively ceil(-eps)
See above comments - looks like I missed a couple things when I hit approve
|
@eric-wieser This completely slipped my mind! I'm back, and it will be fixed in a couple of days. |
|
@eric-wieser Fixed, and now all green! |
| return -1; | ||
| } | ||
|
|
||
| val_is_zero = PyObject_RichCompareBool(val, zero, Py_EQ); |
There was a problem hiding this comment.
You need to check that this doesn't return -1, indicating an error occurred. Same above.
|
Thanks @eric-wieser, fixed! |
|
Minor style nits, but easiest to fix them myself. Thanks for the perseverance - I'll leave this for a couple days to see if there are any more comments |
|
@eric-wieser Just a ping. |
|
Thanks for the perseverance, @Licht-T - this will be in 1.15 |
This closes #10206.
The problem is
Where the step size becomes infinite.