8000 BUG: Fix strange behavior of infinite-step-size/underflow-case in arange by Licht-T · Pull Request #10263 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Jun 4, 2018
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
671a224
BUG: Fix strange behavior of infinite step size in arange
Licht-T Dec 23, 2017
825c681
TST: Add tests for np.arange with inf step
Licht-T Dec 23, 2017
4af6233
Merge remote-tracking branch 'original/master' into fix-strange-infin…
Licht-T Dec 23, 2017
2607ad7
Fix build error on Windows
Licht-T Dec 23, 2017
123da27
Fix unnecessary calculation
Licht-T Dec 23, 2017
e2cc3e6
Use npy_signbit
Licht-T Dec 23, 2017
d4d2758
Add commnt
Licht-T Dec 24, 2017
2827c66
Use int as bool, not intp
Licht-T Dec 27, 2017
0b60753
Dereference the leaked PyObject
Licht-T Dec 27, 2017
41cd4e1
Add zero PyObject pointer null-check
Licht-T Dec 28, 2017
12d020c
Change the order of pointer initialization
8000 Licht-T Dec 29, 2017
c5f589d
TST: Add tests for np.arange with underflow cases
Licht-T Dec 29, 2017
abc1686
Merge branch 'master' into fix-strange-infinite-step-arange
Licht-T May 2, 2018
514c8ec
Remove the assigning a double value to a integer variable
Licht-T May 2, 2018
d68afb3
Add `PyObject_RichCompareBool` success check
Licht-T May 4, 2018
2740bc9
STY: Fix whitespace
eric-wieser May 31, 2018
04a85b4
STY: Fix whitespace
eric-wieser May 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 41 additions & 7 deletions numpy/core/src/multiarray/ctors.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2968,11 +2968,25 @@ PyArray_Arange(double start, double stop, double step, int type_num)
PyArray_ArrFuncs *funcs;
PyObject *obj;
int ret;
double delta, tmp_len;
NPY_BEGIN_THREADS_DEF;

length = _arange_safe_ceil_to_intp((stop - start)/step);
if (error_converting(length)) {
return NULL;
delta = stop - start;
tmp_len = delta/step;

if (tmp_len == 0.0 && delta != 0.0) {
Copy link
Member

Choose a reason for hiding this comment

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

This needs a comment explaining why it is a special case

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

Copy link
Contributor Author
@Licht-T Licht-T Dec 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

if (npy_signbit(tmp_len)) {
length = -1.0;
}
else {
length = 1.0;
Copy link
Member

Choose a reason for hiding this comment

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

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

}
}
else {
length = _arange_safe_ceil_to_intp(tmp_len);
if (error_converting(length)) {
return NULL;
}
}

if (length <= 0) {
Expand Down Expand Up @@ -3035,7 +3049,8 @@ PyArray_Arange(double start, double stop, double step, int type_num)
static npy_intp
_calc_length(PyObject *start, PyObject *stop, PyObject *step, PyObject **next, int cmplx)
{
npy_intp len, tmp;
npy_intp len, tmp, next_is_nonzero, val_is_zero;
PyObject *zero = PyInt_FromLong(0);
PyObject *val;
double value;

Expand All @@ -3049,12 +3064,19 @@ _calc_length(PyObject *start, PyObject *stop, PyObject *step, PyObject **next, i
}
return -1;
Copy link
Member

Choose a reason for hiding this comment

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

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

}

next_is_nonzero = PyObject_RichCompareBool(*next, zero, Py_NE);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

val = PyNumber_TrueDivide(*next, step);
Py_DECREF(*next);
*next = NULL;

if (!val) {
return -1;
Copy link
Member

Choose a reason for hiding this comment

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

zero is leaked here

}

val_is_zero = PyObject_RichCompareBool(val, zero, Py_EQ);
Copy link
Member

Choose a reason for hiding this comment

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

Should be an int, not an intp.

Copy link
Member
@eric-wieser eric-wieser May 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Py_DECREF(zero);

if (cmplx && PyComplex_Check(val)) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

See #10332.

value = PyComplex_RealAsDouble(val);
if (error_converting(value)) {
Expand Down Expand Up @@ -3083,11 +3105,23 @@ _calc_length(PyObject *start, PyObject *stop, PyObject *step, PyObject **next, i
if (error_converting(value)) {
return -1;
}
len = _arange_safe_ceil_to_intp(value);
if (error_converting(len)) {
return -1;

if (val_is_zero && next_is_nonzero) {
if (npy_signbit(value)) {
len = -1.0;
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be 0. as it is effectively ceil(-eps)

}
else {
len = 1.0;
Copy link
Member

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

}
}
else {
len = _arange_safe_ceil_to_intp(value);
if (error_converting(len)) {
return -1;
}
}
}

if (len > 0) {
*next = PyNumber_Add(start, step);
if (!*next) {
Expand Down
17 changes: 17 additions & 0 deletions numpy/core/tests/test_regression.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,23 @@ def test_arange_endian(self):
x = np.arange(10, dtype='>f8')
assert_array_equal(ref, x)

def test_arange_inf_step(self):
ref = np.arange(0, 1, 10)
x = np.arange(0, 1, np.inf)
assert_array_equal(ref, x)

ref = np.arange(0, 1, -10)
x = np.arange(0, 1, -np.inf)
assert_array_equal(ref, x)

ref = np.arange(0, -1, -10)
x = np.arange(0, -1, -np.inf)
assert_array_equal(ref, x)

ref = np.arange(0, -1, 10)
x = np.arange(0, -1, np.inf)
assert_array_equal(ref, x)

def test_argmax(self):
# Ticket #119
a = np.random.normal(0, 1, (4, 5, 6, 7, 8))
Expand Down
0