-
-
Notifications
You must be signed in to change notification settings - Fork 32.8k
gh-117999: Fix small integer powers of complex numbers #124243
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
f2280be
5b5320a
1304423
bc4adfd
4b8535d
e876273
bb748c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Fix calculation of powers of complex numbers. Small integer powers now produce correct sign of zero components. Negative powers of infinite numbers now evaluate to zero instead of NaN. | ||
Powers of infinite numbers no longer raise OverflowError. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -333,23 +333,31 @@ _Py_c_pow(Py_complex a, Py_complex b) | |
r.real = len*cos(phase); | ||
r.imag = len*sin(phase); | ||
|
||
_Py_ADJUST_ERANGE2(r.real, r.imag); | ||
if (isfinite(a.real) && isfinite(a.imag) | ||
&& isfinite(b.real) && isfinite(b.imag)) | ||
{ | ||
_Py_ADJUST_ERANGE2(r.real, r.imag); | ||
} | ||
} | ||
return r; | ||
} | ||
|
||
static Py_complex | ||
c_powu(Py_complex x, long n) | ||
{ | ||
Py_complex r, p; | ||
long mask = 1; | ||
r = c_1; | ||
p = x; | ||
while (mask > 0 && n >= mask) { | ||
if (n & mask) | ||
r = _Py_c_prod(r,p); | ||
mask <<= 1; | ||
p = _Py_c_prod(p,p); | ||
assert(n > 0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you want to make this function a bit more natural, I would suggest having There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my patch above: #124243 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I remember this one. I think we can just keep special-casing n == 0 without actually wondering about the cutoff limit. A generic square-and-multiply algorithm is fine IMO. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, it's possible to find base and exponent, when binary exponentiation is worse. E.g. on this branch, if we omit cutoff, on my system I have:
(Though, both results looks to be a garbage.) |
||
while ((n & 1) == 0) { | ||
x = _Py_c_prod(x, x); | ||
n >>= 1; | ||
} | ||
Py_complex r = x; | ||
n >>= 1; | ||
while (n) { | ||
x = _Py_c_prod(x, x); | ||
if (n & 1) { | ||
r = _Py_c_prod(r, x); | ||
} | ||
n >>= 1; | ||
skirpichev marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+348
to
+360
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For me, your tests works fine with my patch from #118000. Here it is, with slightly adjusted formatting (and using new _Py_rc_quot): diff --git a/Objects/complexobject.c b/Objects/complexobject.c
index b66ebe131a..9dc5d22b37 100644
--- a/Objects/complexobject.c
+++ b/Objects/complexobject.c
@@ -333,19 +333,26 @@ _Py_c_pow(Py_complex a, Py_complex b)
r.real = len*cos(phase);
r.imag = len*sin(phase);
- _Py_ADJUST_ERANGE2(r.real, r.imag);
+ if (isfinite(a.real) && isfinite(a.imag)
+ && isfinite(b.real) && isfinite(b.imag))
+ {
+ _Py_ADJUST_ERANGE2(r.real, r.imag);
+ }
}
return r;
}
+#define INT_EXP_CUTOFF 100
+
static Py_complex
c_powu(Py_complex x, long n)
{
- Py_complex r, p;
+ Py_complex p = x, r = n-- ? p : c_1;
long mask = 1;
- r = c_1;
- p = x;
- while (mask > 0 && n >= mask) {
+ assert(-1 <= n);
+ assert(n < INT_EXP_CUTOFF);
+ while (n >= mask) {
+ assert(mask > 0);
if (n & mask)
r = _Py_c_prod(r,p);
mask <<= 1;
@@ -357,11 +364,10 @@ c_powu(Py_complex x, long n)
static Py_complex
c_powi(Py_complex x, long n)
{
- if (n > 0)
- return c_powu(x,n);
+ if (n >= 0)
+ return c_powu(x, n);
else
- return _Py_c_quot(c_1, c_powu(x,-n));
-
+ return _Py_rc_quot(1.0, c_powu(x, -n));
}
double
@@ -751,9 +757,13 @@ complex_pow(PyObject *v, PyObject *w, PyObject *z)
errno = 0;
// Check whether the exponent has a small integer value, and if so use
// a faster and more accurate algorithm.
- if (b.imag == 0.0 && b.real == floor(b.real) && fabs(b.real) <= 100.0) {
+ if (b.imag == 0.0 && b.real == floor(b.real)
+ && fabs(b.real) <= INT_EXP_CUTOFF)
+ {
p = c_powi(a, (long)b.real);
- _Py_ADJUST_ERANGE2(p.real, p.imag);
+ if (isfinite(a.real) && isfinite(a.imag)) {
+ _Py_ADJUST_ERANGE2(p.real, p.imag);
+ }
}
else {
p = _Py_c_pow(a, b); (asserts are outcome of review, I'm not sure they are useful here.) What do you think? |
||
} | ||
return r; | ||
} | ||
|
@@ -358,10 +366,11 @@ static Py_complex | |
c_powi(Py_complex x, long n) | ||
{ | ||
if (n > 0) | ||
return c_powu(x,n); | ||
return c_powu(x, n); | ||
else if (n < 0) | ||
return _Py_rc_quot(1.0, c_powu(x, -n)); | ||
else | ||
return _Py_c_quot(c_1, c_powu(x,-n)); | ||
|
||
return c_1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Took me a while to remember that this was 1 + 0j (maybe rename that variable in a separate issue, e.g., There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This now used just in one place. If we need this branch - it's better to inline struct value here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is the case, then yes, let's inline it. |
||
} | ||
|
||
double | ||
|
@@ -737,7 +746,9 @@ complex_pow(PyObject *v, PyObject *w, PyObject *z) | |
// a faster and more accurate algorithm. | ||
if (b.imag == 0.0 && b.real == floor(b.real) && fabs(b.real) <= 100.0) { | ||
p = c_powi(a, (long)b.real); | ||
_Py_ADJUST_ERANGE2(p.real, p.imag); | ||
if (isfinite(a.real) && isfinite(a.imag)) { | ||
_Py_ADJUST_ERANGE2(p.real, p.imag); | ||
} | ||
skirpichev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
else { | ||
p = _Py_c_pow(a, b); | ||
|
Uh oh!
There was an error while loading. Please reload this page.