python / cpython

The Python programming language
https://www.python.org/
Other
60.94k stars 29.41k forks source link

_Py_c_pow() should adjust errno on error conditions #119771

Open skirpichev opened 1 month ago

skirpichev commented 1 month ago

Bug report

Bug description:

Right now we do this after invocation of the function or it's optimized alternative (for small integers). That has advantage as - IIUC - both algorithms may trigger error condition. On another hand, behaviour of the public C API function _Py_c_pow() (used in the CPython codebase only for complex_pow()) will differ from the pure-python pow()...

Other similar functions (complex_div() and complex_abs()) leave setting errno to corresponding C-API function.

My proposal: move _Py_ADJUST_ERANGE2() call to _Py_c_pow() and c_powi(). If that does make sense I'll provide a patch.

CPython versions tested on:

CPython main branch

Operating systems tested on:

No response

Linked PRs

vstinner commented 1 month ago

My proposal: move _Py_ADJUST_ERANGE2() call to _Py_c_pow() and c_powi(). If that does make sense I'll provide a patch.

It makes sense to me. You can add tests to Lib/test/test_capi/test_complex.py.

skirpichev commented 1 month ago

I'm working on this. Things are worse than I expect: algorithm for small integer exponents produces (nan+nanj), where _Py_c_pow() - overflows.

a patch ```diff diff --git a/Objects/complexobject.c b/Objects/complexobject.c index 59c84f1359..951a161fb5 100644 --- a/Objects/complexobject.c +++ b/Objects/complexobject.c @@ -152,6 +152,8 @@ _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); } return r; } @@ -175,11 +177,16 @@ 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); - else - return _Py_c_quot(c_1, c_powu(x,-n)); + Py_complex r; + if (n > 0) { + r = c_powu(x, n); + } + else { + r = _Py_c_quot(c_1, c_powu(x,-n)); + } + _Py_ADJUST_ERANGE2(r.real, r.imag); + return r; } double @@ -551,7 +558,6 @@ complex_pow(PyObject *v, PyObject *w, PyObject *z) p = _Py_c_pow(a, b); } - _Py_ADJUST_ERANGE2(p.real, p.imag); if (errno == EDOM) { PyErr_SetString(PyExc_ZeroDivisionError, "0.0 to a negative or complex power"); ```
>>> pow(1e300+1j, 90.1)
Traceback (most recent call last):
  File "<python-input-0>", line 1, in <module>
    pow(1e300+1j, 90.1)
    ~~~^^^^^^^^^^^^^^^^
OverflowError: complex exponentiation
>>> pow(1e300+1j, 90)
(nan+nanj)
vstinner commented 1 month ago

I'm working on this. Things are worse than I expect: algorithm for small integer exponents produces (nan+nanj), where _Py_c_pow() - overflows.

Usually for numbers in Python, in case of doubt, correctness matters more than performance.

skirpichev commented 1 month ago

Well, do you suggest to wipe out c_powi()?:) That could solve also #117999, but performance impact is measurable:

$ python3.12 -m timeit -s 'a, b =1+1j, 10' 'pow(a, b)'
1000000 loops, best of 5: 230 nsec per loop
$ python3.12 -m timeit -s 'a, b =1+1j, 10.000001' 'pow(a, b)'
500000 loops, best of 5: 560 nsec per loop
vstinner commented 1 month ago

You can catch the overflow using something like that:

diff --git a/Objects/complexobject.c b/Objects/complexobject.c
index 59c84f1359..c90c6805b7 100644
--- a/Objects/complexobject.c
+++ b/Objects/complexobject.c
@@ -164,10 +164,19 @@ c_powu(Py_complex x, long n)
     r = c_1;
     p = x;
     while (mask > 0 && n >= mask) {
-        if (n & mask)
+        if (n & mask) {
             r = _Py_c_prod(r,p);
+            if (!isfinite(p.real) || !isfinite(p.imag)) {
+                errno = ERANGE;
+                break;
+            }
+        }
         mask <<= 1;
         p = _Py_c_prod(p,p);
+        if (!isfinite(p.real) || !isfinite(p.imag)) {
+            errno = ERANGE;
+            break;
+        }
     }
     return r;
 }
@@ -551,7 +560,9 @@ complex_pow(PyObject *v, PyObject *w, PyObject *z)
         p = _Py_c_pow(a, b);
     }

-    _Py_ADJUST_ERANGE2(p.real, p.imag);
+    if (errno == 0) {
+        _Py_ADJUST_ERANGE2(p.real, p.imag);
+    }
     if (errno == EDOM) {
         PyErr_SetString(PyExc_ZeroDivisionError,
                         "0.0 to a negative or complex power");
skirpichev commented 1 month ago

Hmm, shouldn't errno set _Py_c_prod() in this case?

skirpichev commented 1 month ago

See https://github.com/python/cpython/issues/120010

skirpichev commented 1 month ago

PR is ready for review: https://github.com/python/cpython/pull/120256