Skip to content
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

_Py_c_pow() should adjust errno on error conditions #119771

Closed
skirpichev opened this issue May 30, 2024 · 9 comments
Closed

_Py_c_pow() should adjust errno on error conditions #119771

skirpichev opened this issue May 30, 2024 · 9 comments
Labels
type-bug An unexpected behavior, bug, or error

Comments

@skirpichev
Copy link
Member

skirpichev commented May 30, 2024

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

@skirpichev skirpichev added the type-bug An unexpected behavior, bug, or error label May 30, 2024
@vstinner
Copy link
Member

vstinner commented Jun 3, 2024

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
Copy link
Member Author

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 --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
Copy link
Member

vstinner commented Jun 3, 2024

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
Copy link
Member Author

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
Copy link
Member

vstinner commented Jun 3, 2024

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
Copy link
Member Author

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

@skirpichev
Copy link
Member Author

See #120010

skirpichev added a commit to skirpichev/cpython that referenced this issue Jun 8, 2024
Before we did this in complex_pow() and behaviour of the public C API
function ``_Py_c_pow()`` was different from the pure-python pow().
@skirpichev
Copy link
Member Author

PR is ready for review: #120256

vstinner pushed a commit that referenced this issue Sep 18, 2024
Before we did this in complex_pow() and behavior of the public C API
function _Py_c_pow() was different from the pure-python pow().
@vstinner
Copy link
Member

Implemented by the change 8a284e1.

savannahostrowski pushed a commit to savannahostrowski/cpython that referenced this issue Sep 22, 2024
Before we did this in complex_pow() and behavior of the public C API
function _Py_c_pow() was different from the pure-python pow().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

2 participants