BUG: Fix strange behavior of infinite-step-size/underflow-case in arange #10263

Licht-T · 2017-12-23T04:06:32Z

This closes #10206.

The problem is

>>> np.arange(0, 1, 1e10)
array([0.])
>>> np.arange(0, 1, 1e100)
array([0.])
>>> np.arange(0, 1, 1e1000)  # overflows
array([], dtype=float64)
>>> np.arange(0, 1, np.inf)
array([], dtype=float64)

Where the step size becomes infinite.

…ite-step-arange

ghost · 2017-12-23T04:50:19Z

numpy/core/src/multiarray/ctors.c

-            return -1;
+
+        if (val_is_zero && next_is_nonzero) {
+            if (signbit(value)) {


Use npy_signbit.

Licht-T · 2017-12-23T06:26:01Z

Thanks @xoviat, fixed!

eric-wieser · 2017-12-23T21:42:49Z

numpy/core/src/multiarray/ctors.c

+    delta = stop - start;
+    tmp_len = delta/step;
+
+    if (tmp_len == 0.0 && delta != 0.0) {


This needs a comment explaining why it is a special case

Isn't this only satisfied if step is inf anyway?

@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)

Licht-T · 2017-12-24T13:23:08Z

@eric-wieser Added comments.

charris · 2017-12-25T23:13:23Z

numpy/core/src/multiarray/ctors.c

+    delta = stop - start;
+    tmp_len = delta/step;
+
+    // Underflow and divide-by-inf check


Don't use C++ comments, use /* ... */.

Thanks @charris, fixed!

eric-wieser · 2017-12-27T06:20:35Z

numpy/core/src/multiarray/ctors.c

    if (!val) {
        return -1;
    }
+
+    val_is_zero = PyObject_RichCompareBool(val, zero, Py_EQ);


Should be an int, not an intp.

eric-wieser · 2017-12-27T06:22:51Z

numpy/core/src/multiarray/ctors.c

@@ -3049,12 +3065,19 @@ _calc_length(PyObject *start, PyObject *stop, PyObject *step, PyObject **next, i
        }
        return -1;
    }
+
+    next_is_nonzero = PyObject_RichCompareBool(*next, zero, Py_NE);


Could use PyObject_IsTrue, but that might be worse on unusual types

@eric-wieser IMO, PyObject_RichCompareBool is better because this is clear what we do.

eric-wieser · 2017-12-27T06:23:48Z

numpy/core/src/multiarray/ctors.c

    val = PyNumber_TrueDivide(*next, step);
    Py_DECREF(*next);
    *next = NULL;
+
    if (!val) {
        return -1;


zero is leaked here

Licht-T · 2017-12-27T16:23:41Z

Thanks @eric-wieser, fixed!

eric-wieser · 2017-12-27T16:33:46Z

numpy/core/src/multiarray/ctors.c

@@ -3049,12 +3066,20 @@ _calc_length(PyObject *start, PyObject *stop, PyObject *step, PyObject **next, i
        }
        return -1;


Need to DECREF zero here too - or better yet, just allocate it further down.

eric-wieser · 2017-12-27T16:40:03Z

numpy/core/src/multiarray/ctors.c

@@ -3036,7 +3051,9 @@ static npy_intp
 _calc_length(PyObject *start, PyObject *stop, PyObject *step, PyObject **next, int cmplx)
 {
    npy_intp len, tmp;
+    PyObject *zero = PyInt_FromLong(0);


Really you should check the return value is not NULL here too.

Licht-T · 2017-12-28T00:34:20Z

@eric-wieser Fixed!

Licht-T · 2017-12-28T00:42:20Z

Also, I need to add some underflow-case tests.

eric-wieser · 2017-12-28T07:07:37Z

numpy/core/src/multiarray/ctors.c

    *next = PyNumber_Subtract(stop, start);
    if (!(*next)) {
+        Py_DECREF(zero);


If you move the allocation of zero down below this if, then you don't need to clear it up here

@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.

and set NULL to *next when the zero allocation failed.

You already need to do this

eric-wieser · 2017-12-28T07:10:06Z

Did you mean to refer to #10271 in the PR description? Seems unrelated to me

Licht-T · 2017-12-28T11:07:39Z

@eric-wieser I did not refer to that issue. Maybe @charris added this.

Did you mean to refer to #10271 in the PR description? Seems unrelated to me

Licht-T · 2017-12-29T07:10:42Z

Thanks @eric-wieser, fixed!

Licht-T · 2017-12-29T16:07:00Z

Added the test for some underflow cases

eric-wieser · 2018-01-05T18:14:51Z

numpy/core/src/multiarray/ctors.c

+
+    val_is_zero = PyObject_RichCompareBool(val, zero, Py_EQ);
+    Py_DECREF(zero);
+
    if (cmplx && PyComplex_Check(val)) {


Trying to work out if this section needs a fix too, but I can't work out what this is even trying to do.

@eric-wieser Me t 10000 oo. I don't know why the complex case exists. This is none-sense as math.

See #10332.

eric-wieser · 2018-01-05T18:35:15Z

Can you extract double _ceildiv(double, double) and PyObject * _ceildiv_O(PyObject *, PyObject *) helper functions here, that handles all these corner cases for us?

eric-wieser

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.

eric-wieser · 2018-01-06T18:58:59Z

numpy/core/src/multiarray/ctors.c

+            length = -1.0;
+        }
+        else {
+            length = 1.0;


nit: should be 1, as this is an integer, not double, variable. Same above.

eric-wieser · 2018-01-06T19:02:51Z

numpy/core/src/multiarray/ctors.c

+                len = -1.0;
+            }
+            else {
+                len = 1.0;


Same here for int vs double

eric-wieser · 2018-01-06T19:04:57Z

numpy/core/src/multiarray/ctors.c

+        /* Underflow and divide-by-inf check */
+        if (val_is_zero && next_is_nonzero) {
+            if (npy_signbit(value)) {
+                len = -1.0;


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

Licht-T · 2018-05-02T05:10:14Z

@eric-wieser This completely slipped my mind! I'm back, and it will be fixed in a couple of days.

Licht-T · 2018-05-02T15:34:35Z

@eric-wieser Fixed, and now all green!

eric-wieser · 2018-05-03T05:00:08Z

numpy/core/src/multiarray/ctors.c

        return -1;
    }
+
+    val_is_zero = PyObject_RichCompareBool(val, zero, Py_EQ);


You need to check that this doesn't return -1, indicating an error occurred. Same above.

Licht-T · 2018-05-04T15:12:15Z

Thanks @eric-wieser, fixed!

eric-wieser · 2018-05-31T06:35:40Z

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

charris · 2018-06-02T17:18:41Z

@eric-wieser Just a ping.

eric-wieser · 2018-06-04T05:10:53Z

Thanks for the perseverance, @Licht-T - this will be in 1.15

Licht-T added 5 commits December 23, 2017 13:03

BUG: Fix strange behavior of infinite step size in arange

671a224

TST: Add tests for np.arange with inf step

825c681

Merge remote-tracking branch 'original/master' into fix-strange-infin…

4af6233

…ite-step-arange

Fix build error on Windows

2607ad7

Fix unnecessary calculation

123da27

ghost reviewed Dec 23, 2017

View reviewed changes

numpy/core/src/multiarray/ctors.c Outdated

return -1;

if (val_is_zero && next_is_nonzero) {

if (signbit(value)) {

Copy link

ghost Dec 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use npy_signbit.

Use npy_signbit

e2cc3e6

eric-wieser reviewed Dec 23, 2017

View reviewed changes

charris reviewed Dec 25, 2017

View reviewed changes

charris added 00 - Bug component: numpy._core labels Dec 25, 2017

Add commnt

d4d2758

Licht-T force-pushed the fix-strange-infinite-step-arange branch from 96b261d to d4d2758 Compare December 26, 2017 00:19

eric-wieser reviewed Dec 27, 2017

View reviewed changes

Use int as bool, not intp

2827c66

eric-wieser reviewed Dec 27, 2017

View reviewed changes

Licht-T added 2 commits December 28, 2017 09:32

Dereference the leaked PyObject

0b60753

Add zero PyObject pointer null-check

729A

41cd4e1

Licht-T force-pushed the fix-strange-infinite-step-arange branch from 5f6b735 to 41cd4e1 Compare December 28, 2017 00:32

Licht-T changed the title ~~BUG: Fix strange behavior of infinite step size in arange~~ BUG: Fix strange behavior of infinite-step-size/underflow-case in arange Dec 28, 2017

eric-wieser reviewed Dec 28, 2017

View reviewed changes

TST: Add tests for np.arange with underflow cases

c5f589d

eric-wieser reviewed Jan 5, 2018

View reviewed changes

eric-wieser previously approved these changes Jan 6, 2018

View reviewed changes

eric-wieser reviewed Jan 6, 2018

View reviewed changes

numpy/core/src/multiarray/ctors.c Outdated

len = -1.0;

}

else {

len = 1.0;

Copy link

Member

eric-wieser Jan 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here for int vs double

eric-wieser reviewed Jan 6, 2018

View reviewed changes

eric-wieser added the 55 - Needs work label Apr 20, 2018

eric-wieser mentioned this pull request Apr 24, 2018

varying Errors while changing the type of 0 as a step in arange #10496

Open

Licht-T added 2 commits May 2, 2018 18:05

Merge branch 'master' into fix-strange-infinite-step-arange

abc1686

Remove the assigning a double value to a integer variable

514c8ec

eric-wieser reviewed May 3, 2018

View reviewed changes

Add PyObject_RichCompareBool success check

d68afb3

eric-wieser added 2 commits May 30, 2018 23:33

STY: Fix whitespace

2740bc9

STY: Fix whitespace

04a85b4

eric-wieser approved these changes May 31, 2018

View reviewed changes

eric-wieser removed the 55 - Needs work label May 31, 2018

eric-wieser added this to the 1.15.0 release milestone May 31, 2018

eric-wieser merged commit abeb96d into numpy:master Jun 4, 2018

charris mentioned this pull request Nov 13, 2020

BUG: Using start/step as keywords only has incorrect behaviour #17764

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fix strange behavior of infinite-step-size/underflow-case in arange #10263

BUG: Fix strange behavior of infinite-step-size/underflow-case in arange #10263

		@@ -3049,12 +3066,20 @@ _calc_length(PyObject start, PyObject stop, PyObject step, PyObject *next, i
		}
		return -1;

BUG: Fix strange behavior of infinite-step-size/underflow-case in arange #10263

BUG: Fix strange behavior of infinite-step-size/underflow-case in arange #10263

Conversation

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

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