gh-153144: Avoid checking errno for atan2 - #153148
Conversation
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
If this PR is merged, it looks to me like #146402 can (and should) be reverted. |
skirpichev
left a comment
skirpichev
left a comment
There was a problem hiding this comment.
No, I doubt that this is a right approach. We must handle error in same way for all libm functions.
Perhaps, m_atan2() (like m_log1p() we have currently) could be restored (see #122681) to workaround broken platform functions.
The mathmodule currently has 3 ways of handling 1-argument libm functions:
Maybe it's not so bad to have |
|
Perhaps, we could modify math_2() helper to check that errno!=EDOM, if inputs and output are finite. When domain error occurs - result should be nan. But I think that a little wrapper for libm's atan2 is better, if we are going to add some workaround for the given issue. |
skirpichev
left a comment
skirpichev
left a comment
There was a problem hiding this comment.
Please revert unrelated changes.
|
@vstinner, can you look at this at your convenience? |
| double phi; | ||
|
|
||
| errno = 0; | ||
| phi = atan2(z.imag, z.real); /* should not cause any exception */ |
There was a problem hiding this comment.
Please add comment: // gh-153144: ignore atan2() errno on purpose.
| if (y == -1.0 && PyErr_Occurred()) { | ||
| return NULL; | ||
| } | ||
| r = (*func)(x, y); |
There was a problem hiding this comment.
Please add comment: // ignore errno on purpose.
| "Return the arc tangent (measured in radians) of x.\n\n" | ||
| "The result is between -pi/2 and pi/2.") | ||
| FUNC2(atan2, atan2, | ||
| FUNC2NE(atan2, atan2, |
There was a problem hiding this comment.
Please add comment: // gh-153144: Ignore atan2() and atan2pi() errno on purpose.
| errno = 0; | ||
| phi = atan2(z.imag, z.real); /* should not cause any exception */ | ||
| if (errno != 0) | ||
| return math_error(); |
There was a problem hiding this comment.
I expected a test_cmath failure when this code path is removed. Is it because glibc math library doesn't errno in this case?
There was a problem hiding this comment.
Yes. With the exception of the Intel and Solaris math libraries, errno is not set in this case by any math library that Python cares about. I say this because test_phase in Lib/test/test_cmath.py has asserted that return values are correct since Python 3.14, and nobody has complained. If errno were set by the C math library, the unittest would fail with ValueError: math domain error.
🌱 Some math libraries (e.g., musl) don't set errno for anything, so Python cannot rely on errno for detecting overflow or invalid. I would think that errno checking can be removed everywhere....
This reverts commit 8e1469c.
3 participants
The C23 standard states that for
atan2andatan2pi:Since Python should not raise ValueError in either of these cases (i.e., when both arguments are zero or when the computation underflows), this PR avoids checking
errnowhen calling these trig functions. As a bonus,math.atan2()is about 4% faster.math.atan2(0.0, 0.0)andcmath.phase(0.0)using icx #153144The statement about range error in the standard should, I think, be interpreted as: