gh-117999: use generic algorithm in complex_pow() if base has special components#123283
gh-117999: use generic algorithm in complex_pow() if base has special components#123283skirpichev wants to merge 9 commits into
Conversation
Sorry, something went wrong.
|
I had some thoughts on that matter. For small integer exponents $b = \Re(b) + \Im(b)\cdot\jmath$ with $\Im(b) = 0$ and $\Re(b) \in \mathbb{Z}$, we would now call vabs = hypot(a.real,a.imag);
len = pow(vabs,b.real);
at = atan2(a.imag, a.real);
phase = at*b.real;
/* if (b.imag != 0.0) {
len /= exp(at*b.imag);
phase += b.imag*log(vabs);
} */ // ignored since b.imag == 0
r.real = len*cos(phase);
r.imag = len*sin(phase);Let $a = r(\cos \phi + \jmath \sin \phi)$ and $b = n\in\mathbb{Z}$. De Moivre's formula tells me that $a^{n} = r^{n}(\cos n\phi + \jmath \sin n\phi)$. So, we could technically have an alternative path for the case $n = 2k$ since we can directly compute I don't know whether the costly computation is due to |
Sorry, something went wrong.
Yeah, below are tests copied from #118000 (comment) with few more with odd exponents: With specialized code (main): Without: |
Sorry, something went wrong.
Can I have the numbers for odd exponents without as well please? |
Sorry, something went wrong.
|
Sure, comment was updated. As you can see, results aren't biased to even/odd integers. |
Sorry, something went wrong.
|
Do you think it's worth checking the even case for a fast path or do you think we should just live with those this loss of performance? (personally, in this case, I'm for correctness rather than speed). |
Sorry, something went wrong.
|
I think it not worth. Certainly, this can't restore speed for small integers (for even case). |
Sorry, something went wrong.
picnixz
left a comment
There was a problem hiding this comment.
I'm sorry but I approved without commenting ...
Sorry, something went wrong.
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
picnixz
left a comment
There was a problem hiding this comment.
As I said, I'm in favor of correctness rather than speed in this case. But since #60200 is still under discussion, I'll be interested in hearing from @mdickinson and @serhiy-storchaka for this one.
Sorry, something went wrong.
|
Can you update your PR? There is now a conflict (with the ERANGE fix). |
Sorry, something went wrong.
|
So Python has its own cpow() implementation: Py_complex
_Py_c_pow(Py_complex a, Py_complex b)
{
Py_complex r;
double vabs,len,at,phase;
if (b.real == 0. && b.imag == 0.) {
r.real = 1.;
r.imag = 0.;
}
else if (a.real == 0. && a.imag == 0.) {
if (b.imag != 0. || b.real < 0.)
errno = EDOM;
r.real = 0.;
r.imag = 0.;
}
else {
vabs = hypot(a.real,a.imag);
len = pow(vabs,b.real);
at = atan2(a.imag, a.real);
phase = at*b.real;
if (b.imag != 0.0) {
len /= exp(at*b.imag);
phase += b.imag*log(vabs);
}
r.real = len*cos(phase);
r.imag = len*sin(phase);
_Py_ADJUST_ERANGE2(r.real, r.imag);
}
return r;
}Can we reuse the cpow() of the C library instead? A C11 compiler is now required to build CPython: https://docs.python.org/dev/using/configure.html#build-requirements |
Sorry, something went wrong.
Done.
Yet we don't require optional C11 features, cpow() - is from Annex G. Not even all compilers implement one (mvsc for example, though it has something similar). And I'm not sure about quality of libm implementations for that part of the standard. After support for C complex in ctypes was merged, I did tests (same as in CMathTests.test_specific_values) of cmath_testcases.txt on top of Debian's 12 default gcc & glibc: to my surprise it was working (modulo two simple failures from changes in C17, now fixed). But perhaps I'm just lucky Linux user. |
Sorry, something went wrong.
|
I'm not sure that I understood correctly the purpose of this change. Is it supposed to fix some corner cases and enhance correctness? The change only adds 2 tests which is small. |
Sorry, something went wrong.
|
Try |
Sorry, something went wrong.
Without the change, I get: With the change, I get: |
Sorry, something went wrong.
Well, yes. See examples in the issue description: >>> z = complex(1,-0.0)
>>> z**2
(1+0j)
>>> z*z
(1-0j)
>>> z**2000 # large powers or non-integer - have own opinion
(1-0j)
>>> >>> z**2.1
(1-0j)
>>> _testcapi._py_c_pow(z, 2)[0] # as well as _Py_c_pow() algorithm
(1-0j)
Agreed. I'll work on this.
That's something, which now is more precise in the main, as an accident. But: >>> _testcapi._py_c_pow(z, 1)[0]
(-1+1.2246467991473532e-16j)
>>> libm.cpow(z, 1) # libm's cpow
(-1+1.2246467991473532e-16j) |
Sorry, something went wrong.
|
ok, now it's ready for review again. The fix now is restricted to arguments with special components in the base. That means, e.g. we can get wrong signs when special components appear in intermediate computations. An example from the issue thread: >>> import _testcapi
>>> z = -1-1j
>>> _testcapi._py_c_pow(z, 6)[0]
(4.408728476930473e-15-8.000000000000004j)
>>> z**6
(-0-8j)
>>> z*z*z*z*z*z
-8jThe sign here is just an artifact of special ordering of multiplications, it's different for same base with other exponents: The only possible solution is removing special algorithm for integer exponents. Edit: BTW, I'm in favor of this option (was - previous variant) also on the ground of compatibility with C (which has only cpow() function). The special algorithm for integer exponents introduces mixed-mode arithmetic for powers, but inconsistently: only for small integer exponents. It's better to have here just CC @picnixz |
Sorry, something went wrong.
edited
LoadingUh oh!
There was an error while loading. Please reload this page.
Copy link Copy MarkdownSorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.