BUG: Guarantee non-zero is one in arctan2 #6346
Conversation
| @@ -210,8 +210,10 @@ double npy_spacing(double x); | |||
| (sizeof (x) == sizeof (long double) ? _npy_signbit_ld (x) \ | |||
| : sizeof (x) == sizeof (double) ? _npy_signbit_d (x) \ | |||
| : _npy_signbit_f (x)) | |||
| #define true_npy_signbit(x) (signbit((x)) != 0 ? 1 : 0) | |||
There was a problem hiding this comment.
signbit is not declared in this branch
|
@juliantaylor Agree. |
|
Is this a regression? Just curious why the RC tag |
|
@njsmith Just to keep it in consideration. Release isn't as strict as blocker and if the fix is simple and obvious I might put it in. |
|
Ahh, I missed that we have a "blockers" versus generic "release" milestone now. Thanks :-) |
|
I assume this has been found by static analysis? as our testsuite would have caught it if there is a used platform where it returns something else than 1. |
|
Updated. |
| @@ -130,7 +130,7 @@ double npy_atan2(double y, double x) | |||
| return npy_atan(y); | |||
| } | |||
|
|
|||
| m = 2 * npy_signbit(x) + npy_signbit(y); | |||
| m = 2 * (signbit((x)) != 0 ? 1 : 0) + (signbit((y)) != 0 ? 1 : 0); | |||
There was a problem hiding this comment.
Need to use npy_signbit here, signbit may not be defined. For instance, I believe it was absent from msvcc prior to 2013. Microsoft C doesn't move very quickly.
|
Updated. |
| @@ -130,7 +130,7 @@ double npy_atan2(double y, double x) | |||
| return npy_atan(y); | |||
| } | |||
|
|
|||
| m = 2 * npy_signbit(x) + npy_s 8000 ignbit(y); | |||
| m = 2 * (npy_signbit((x)) != 0 ? 1 : 0) + (npy_signbit((y)) != 0 ? 1 : 0); | |||
There was a problem hiding this comment.
Actually, IIRC comparison operators are documented to return 1 or 0, so this can be simplified to
m = 2 * (npy_signbit((x)) != 0) + (npy_signbit((y)) != 0);
+----C99:6.5.8 Relational operators-----
6. Each of the operators < (less than), > (greater than), <= (less than or equal to), and >= (greater than or equal to)
shall yield 1 if the specified relation is true and 0 ifit is false.The result has type int.
And the same holds for == and !=. That goes back to K&R.
In numpy/core/src/npymath/npy_math.c.src there is a state machine sequence that assumes signbit returns either a 1 or 0. However, all the online documentation states that it will return either a 0 or a nonzero value, which seems to be determined by the OS. These changes allow the code to work with a zero or a nonzero value.
|
Cool. Updated. |
BUG: Guarantee non-zero is one in arctan2
|
Thanks @ChristopherHogan . |
In numpy/core/src/npymath/npy_math.c.src there is a state machine
sequence that assumes signbit returns either a 1 or 0. However, all
the online documentation states that it will return either a 0 or a
nonzero value, which seems to be determined by the OS. These changes
allow the code to work with a zero or nonzero value.