Skip to content

gh-153144: Avoid checking errno for atan2 - #153148

Open
hpkfft wants to merge 16 commits into
python:mainfrom
hpkfft:atan2
Open

hpkfft wants to merge 16 commits into
python:mainfrom
hpkfft:atan2

Conversation

@hpkfft

@hpkfft hpkfft commented Jul 5, 2026 •

Copy link
Copy Markdown
Contributor

The C23 standard states that for atan2 and atan2pi:

A domain error may occur if both arguments are zero.
A range error occurs if x is positive and nonzero y/x is too close to zero.

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 errno when calling these trig functions. As a bonus, math.atan2() is about 4% faster.


The statement about range error in the standard should, I think, be interpreted as:

A range error occurs if (x is positive) and (y/x is nonzero) and (y/x is too close to zero).

@bedevere-app

bedevere-app Bot commented Jul 5, 2026

Copy link
Copy Markdown

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 skip news label instead.

@hpkfft

hpkfft commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

If this PR is merged, it looks to me like #146402 can (and should) be reverted.

@picnixz
picnixz requested a review from skirpichev July 6, 2026 23:41

@skirpichev skirpichev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hpkfft

hpkfft commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

We must handle error in same way for all libm functions.

The mathmodule currently has 3 ways of handling 1-argument libm functions:

  • Functions that cannot overflow (e.g., acos, atan) use math_1(arg, func, /*can_overflow=*/0, err_msg)
  • Functions that can overflow (e.g., cosh, exp) use math_1(arg, func, /*can_overflow=*/1, err_msg)
  • Functions that are known to set errno properly (e.g., erf, gamma) use math_1a(arg, func, err_msg)

Maybe it's not so bad to have math_2ne for 2-argument functions that are known not to need error checking (e.g., atan2) as well as the current math_2 (for the others).

@skirpichev

Copy link
Copy Markdown
Member

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 skirpichev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert unrelated changes.

Comment thread Modules/mathmodule.c Outdated
Comment thread Modules/mathmodule.c Outdated
@hpkfft

hpkfft commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

@vstinner, can you look at this at your convenience?
I anticipate that this PR will also fix Solaris to return the correct results (#138573), so the relevant math/cmath tests will not need to be skipped on that platform. Please advise whether I should revert #146402 as part of this PR. If so, I'll do that and would then ask you to run on the buildbots.

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this PR is merged, it looks to me like #146402 can (and should) be reverted.

I would prefer to update tests for Solaris in the same PR.

Comment thread Modules/cmathmodule.c Outdated
Comment thread Modules/mathmodule.c Outdated
Comment thread Modules/mathmodule.c
Comment thread Modules/cmathmodule.c
errno = 0;
phi = atan2(z.imag, z.real); /* should not cause any exception */
if (errno != 0)
return math_error();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expected a test_cmath failure when this code path is removed. Is it because glibc math library doesn't errno in this case?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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....

@skirpichev

Copy link
Copy Markdown
Member

You also could use new helper function for copysign.

On another hand, I would prefer just remove errno stuff from math_2(), per #156145. It should be possible for all two-argument functions: atan2, atan2pi, copysign, remainder and fmod (which could utilize same wrapper).

Comment thread Modules/cmathmodule.c Outdated
Comment thread Modules/mathmodule.c Outdated
Comment thread Modules/mathmodule.c Outdated
Comment thread Modules/mathmodule.c
@hpkfft

hpkfft commented Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

You also could use new helper function for copysign.

My thinking is only to do what is necessary to fix the bug (the incorrect results when building with Intel or Solaris math libraries). Then, I would recommend applying this fix to Python 3.14 and 3.15. With that in mind, I don't want to make any enhancements that are not necessary to fix the reported bug.

On another hand, I would prefer just remove errno stuff from math_2(), per #156145. It should be possible for all two-argument functions: atan2, atan2pi, copysign, remainder and fmod (which could utilize same wrapper).

Yes, 156145 can have a more ambitious goal.

Can you trigger the buildbots to run this PR? I am not able to check myself that it works on Solaris. (I do think it will work based on what I read in the Solaris bug report.)

@skirpichev

Copy link
Copy Markdown
Member

My thinking is only to do what is necessary to fix the bug

Then we loose chance of using new helper function for copysign().

Can you trigger the buildbots to run this PR?

!buildbot Solaris

@skirpichev

Copy link
Copy Markdown
Member

!buildbot Solaris

@bedevere-bot

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @skirpichev for commit 5b6d005 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F153148%2Fmerge

The command will test the builders whose names match following regular expression: Solaris

The builders matched are:

  • SPARCv9 Oracle Solaris 11.4 PR

@hpkfft

hpkfft commented Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

Then we loose chance of using new helper function for copysign().

OK, I'll add that commit tomorrow. Since it will be a separate commit, it can easily be reverted (or skipped when squashing all the commits before the final merge). The same is true for the comments Victor requested--they're in their own separate commit.

@hpkfft

hpkfft commented Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

I'm happy to see that the math and cmath tests pass on Solaris.
As requested, I added commit to use FUNC2NE for copysign.

@hpkfft

hpkfft commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Hi @StanFromIreland,

Can you please advise whether I should create a separate bug report for an issue @serhiy-storchaka observed:

Python 3.13.5 (main, Jul 15 2026, 20:25:40) [GCC 14.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import cmath
>>> cmath.phase(complex(1e300, 1e-320))
Traceback (most recent call last):
  File "<python-input-1>", line 1, in <module>
    cmath.phase(complex(1e300, 1e-320))
    ~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^
OverflowError: math range error

or whether #153144 can be updated with this extra information?
This PR, by not checking errno, fixes this bug too (correctly returning 0.0).

Note that #153144 affects the current version of the Intel math library and the Solaris math library, so it's correctly labelled OS-unsupported.
The OverflowError bug affects any math library that sets errno when z.imag/z.real underflows to zero (e.g., GNU).

@vstinner

vstinner commented Sep 9, 2026

Copy link
Copy Markdown
Member

Hum. It's uneasy for me to take a decision on this change. The bug report is about supporting Intel compiler (icx) which sets errno for atan2() and phase(). But the PR also changes cmath.phase() behavior on overflow and underflow of imag/real.

I'm not sure if cmath.phase(complex(1e300, 1e-320)) should raise an OverflowError (current behavior) or just return 0.0. I failed to find the rationale for changing phase() behavior.

@hpkfft

hpkfft commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor Author

But the PR also changes cmath.phase() behavior on overflow and underflow of imag/real.

Python already returns the correct result, pi/2, if imag/real OVERflows (using 3.13 as shipped by Debian):

>>> cmath.phase(complex(1E-320, 1E300))
1.570796326794896

This PR changes cmath.phase() behavior on UNDERflow of imag/real (bug observed in #155527 (comment)).

The C standard in 7.3.9.1 says:

The carg functions compute the argument (also called phase (which is an angle)) of z, with a branch cut along the negative real axis.
The carg functions return the value of the argument in the interval [−π, +π].

Note that there's no possibility of overflow--the result is bounded. (The implementation may or may not actually perform a floating-point division of imag/real, but that's an implementation detail, and whether or not some intermediate calculation overflows must not affect what the user observes.)

There is the possibility of underflow. The infinitely precise result of cmath.phase(complex(1e300, 1e-320)) is very close to zero, but is not exactly zero:

>>> z = gmpy2.mpc(1e300, 1e-320)
>>> z
mpc('1.0000000000000001e+300+9.9998886718268301e-321j')
>>> gmpy2.phase(z)
mpfr('9.9998886718268287e-621')

The result is too small to be represented in double precision, so it underflows to 0.0.

Python does not raise an exception on underflow, only on overflow. For example:

>>> math.exp(-1000)
0.0
>>> math.exp(1000)
Traceback (most recent call last):
  File "<python-input-14>", line 1, in <module>
    math.exp(1000)
    ~~~~~~~~^^^^^^
OverflowError: math range error

I created the bug report because existing tests were failing when building Python with the Intel math library. I made this PR to ignore errno because I could see no reason for checking it, and I believed that not checking it was always correct. I did not stop to consider that the old code (checking errno) led to more Python bugs. After reading Serhiy's comment, I added tests to this PR and updated the NEWS.

@vstinner

Copy link
Copy Markdown
Member

@serhiy-storchaka @skirpichev @picnixz: What is your opinion on changing cmath.phase() behavior?

@skirpichev

Copy link
Copy Markdown
Member

FWIW, I don't think there is a change in behavior: atan2() shouldn't overflow. Underflows in Python are silent.

@picnixz

picnixz commented Sep 10, 2026

Copy link
Copy Markdown
Member

I totally forgot about this PR. Let me read the issue and the history first (and now my speech resembles that of an AI...)

@hpkfft

hpkfft commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

I don't think there is a change in behavior

Well, fixing a bug is a change in behavior. But I know what you mean.

Let me walk through an example (using Python 3.13.5 built to use GNU libm).
Consider the following two complex points:

>>> z1 = complex(+1E300, 1E-320)
>>> z2 = complex(-1E300, 1E-320)

and note that, for both of them, imag/real underflows to zero:

>>> 1E-320 / +1E300
0.0
>>> 1E-320 / -1E300
-0.0

The math module's atan2 gives the correct result for the following:

>>> math.atan2(1E-320, +1E300)
0.0
>>> math.atan2(1E-320, -1E300)
3.141592653589793

In the first case, the result underflows to 0.0. The atan2() function in GNU's libm sets errno = ERANGE to indicate underflow. That is, the infinitely precise result is not exactly zero, but libm rounded the result to zero in order to return a double precision result. Python code in mathmodule.c ignores errno == ERANGE on underflow.

Details

The wrapper math_2() calls is_error(), which does the following:

    else if (errno == ERANGE) {
        if (fabs(x) < 1.5)  // <==== x is the atan2 result, which is 0.0
            result = 0;
        else
            PyErr_SetString(PyExc_OverflowError,
                            "math range error");
    }

In the second case, the infinitely precise result is very close to Pi, and libm rounds the result to three point something in order to return a double precision result. The result did not underflow to zero (nor did it overflow to infinity), so GNU's libm leaves errno unchanged. Python code had set errno = 0 before calling libm's atan2(), so it's still zero. Python returns the result.

Now, we get to the bug.

>>> cmath.phase(z1)
Traceback (most recent call last):
  File "<python-input-12>", line 1, in <module>
    cmath.phase(z1)
    ~~~~~~~~~~~^^^^
OverflowError: math range error

Python calls GNU libm's atan2() exactly as before. The result is 0.0 and errno == ERANGE. Python sees that (errno != 0) and raises OverflowError. There's no code in cmathmodule.c to distinguish underflow from overflow.
[Sure, we could fix this by copying the underflow-ignoring code from mathmodule.c, but that wouldn't fix the issue caused by Intel and Solaris math libraries setting errno = EDOM. Also, it would be unnecessarily slow and confusing.]

In the second case,

>>> cmath.phase(z2)
3.141592653589793

GNU's atan2() returns the correct result and leaves errno unchanged, so it's still zero. Python returns the result.

With this PR:

>>> z1 = complex(+1E300, 1E-320)
>>> z2 = complex(-1E300, 1E-320)

>>> math.atan2(1E-320, +1E300)
0.0
>>> math.atan2(1E-320, -1E300)
3.141592653589793

>>> cmath.phase(z1)
0.0
>>> cmath.phase(z2)
3.141592653589793

@vstinner

Copy link
Copy Markdown
Member

@picnixz: So did you find time to review this change?

@picnixz

picnixz commented Sep 18, 2026

Copy link
Copy Markdown
Member

No I'm sorry. I am quite busy in my daily job and haven't found time for that. I think I will have time on Sunday as I'm flying (or next thursday as I'm flying back). But the past weeks have been very busy (I can only do simple triaging or review).

Now, after a quick glance:

  • On my system (openSUSE, clang, 3.14.5): math.atan2(1e-320, 1e300) gives 0 but cmath.phase(complex(1e300, 1e-320)) raises. This simple discrepancy bothers me already.
  • Now, "A range error occurs if x is positive and nonzero y/x is too close to zero." for atan2(y, x). So I understand this as "a range error occurs", period and that's because we are underflowing, not overflowing (since the domain is bounded as observed).
  • So, consideering the only possibility for having a range error is an underflow, we should raise UnderflowError (if it existed). But it doesn't exist. So it's preferrable that we do nothing (PR behavior).

This makes at least cmath.phase(complex(1e300, 1e-320)) and math.atan2(1e-230, 1e300) consistent. However, I would like to make sure that you document the reason why we ignore errno in this case (that is, atan2() can only raise an optional EDOM, which we can ignore, or an errno due to overflow/underflow. Since overflow is not possible for atan2, we only have underflows and since underflows are silent in Python, we don't care.

Did I catch the issue well? we wouldn't break code though, just fixing things. What I'm worried about however is implementations that really want to raise an EDOM for $(\pm0, \pm0)$. According to Wikipedia:

The C function atan2, and most other computer implementations, are designed to reduce the effort of transforming cartesian to polar coordinates and so always define atan2(0, 0). On implementations without signed zero, or when given positive zero arguments, it is normally defined as 0. It will always return a value in the range [−π, π] rather than raising an error or returning a NaN (Not a Number).

In addition, Annex F says what to return when using (0,0). I would have imagined:

  • If compiler conforms to Annex F -> ignore EDOM as Annex F tells you what to return. We get a value, that's it. Since we don't detect the error handling for math errors, we don't need to care about an optional error being returned right (and whether that error should be considered fatal).
  • If compiler doesn't conform, we forward the EDOM since we don't know whether the returned value is meant to be used or if it's garbage.

But in the second case, we can say it's garbage in-garbage out and it's likely an unsupported compiler (even less supported than icx!) So... I guess we canj ust ignore EDOM?

Did I get it right?

@hpkfft

hpkfft commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

would like to make sure that you document the reason why we ignore errno in this case

Done.

Did I get it right?

Yes. Although, technically, it's about the C math library, not the compiler. (It's possible to mix and match, e.g., use the Intel or Solaris math libraries with GCC.) Perhaps the title of my bug report for icx was too informal.

Note that a math library might not strictly conform to Annex F for many reasons. Here, we only care about the values it returns for atan2(), which I would venture to guess would be correct even if the compiler and math library as a whole cannot claim to be compliant with the entirety of Annex F.
We have tests in Python for phase, which will fail if the C math library's atan2(\pm 0, \pm 0) returns wrong values. If that should happen (for an unsupported build platform), nothing prevents Python from adding some check to configure.ac and providing a m_atan2() and enabling it via #ifndef HAVE_ATAN2 similarly to how it's done for m_atan2pi(). Of course, the better solution would be for the platform's math library to make a fix.
Note that the tests are in Python 3.14, and the only reported failure (prior to my building Python with icx) was on Solaris.

Finally, I would like to suggest that, in my opinion, raising an exception where one should not happen is a serious bug. A Python programmer would not be expected to put cmath.phase() inside a try block. Therefore, this bug in Python leads to a crash.

My understanding is that bugfixes for Python 3.13 must be merged this month.

@skirpichev

Copy link
Copy Markdown
Member

Do not click the "Update branch" button without a good reason because it notifies everyone watching the PR that there are new changes, when there are not, and it uses up limited CI resources.

@picnixz

picnixz commented Oct 2, 2026

Copy link
Copy Markdown
Member

We unfortunately get past 3.13 release and it is not security-only. I am ok with putting it in 3.16 and maybe 3.14/3.15 but I would be interested in having some NumPy's opinion just in case (cc @ngoldbaum). I think fixing the phase/atan2 inconsistency takes precedence over other behavioral change concerns.

@ngoldbaum

Copy link
Copy Markdown
Contributor

Thanks for the ping. NumPy already behaves the way CPython will behave after this PR. The only difference is that NumPy can optionally surface the underflow, and it reports it as an underflow, never as an overflow.

@picnixz

picnixz commented Oct 2, 2026

Copy link
Copy Markdown
Member

Yeah so this matches what users will expect. I am fine with this PR then! thanks for doing it.

@@ -0,0 +1,7 @@
Given a complex number ``z`` for which ``z.imag/z.real`` underflows to zero,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This NEWS entry is waaaay too long IMO. Can we make it a bit shorter?

Comment thread Modules/cmathmodule.c
else
return PyFloat_FromDouble(phi);
phi = atan2(z.imag, z.real);
/* gh-153144: Ignore atan2() errno on purpose since it can optionally be

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: should we clear errno here though? someone calling this function from C may want to know whether errno is appropriately set (or ignored). I would be willing to clear it before calling atan2() but I am not sure it is actually better.

Comment thread Modules/mathmodule.c
{
double x, y, r;
if (!_PyArg_CheckPositional(funcname, nargs, 2, 2))
return NULL;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add braces around the return

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was copied from line 1034 above. Should I add braces to that as well? (That would be my preference.)

Comment thread Modules/mathmodule.c
}

FUNC2(copysign, copysign,
FUNC2NE(copysign, copysign,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh so copysign is also altered? can you explain why here again? is it the same reason? I would appreciate a separate NEWS entry for that as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this at Sergey's request.

Of course, I agree it's the right thing to do, or I would not have done it. The comment for math_2ne() is:

/* variant of math_2, to be used when the function being wrapped is known NOT
   to need error checking (i.e., no overflow, invalid, or divide-by-zero).  */

which certainly applies to copysign. Copying the sign bit cannot possibly cause overflow, invalid, or divide-by-zero. Now that the wrapper math_2ne() exists, it absolutely should be used for copysign. I expect it has a significant performance benefit, and it's also about having a sense of craftsmanship and taking pride in one's work and in the Python code. Given the existence of the wrapper math_2ne(), it would be confusing for somebody looking at copysign to see that math_2() is used.

If you feel copysign deserves its own NEWS entry, maybe it's better to revert the change here and create a separate PR for it? Please let me know what you think, and in particular, if you're inclined to merge a separate PR. If nobody is going to merge it, I don't want to waste my time creating it. Sergey's warning was if I don't add this code to this PR, we lose chance of using new helper function for copysign(). I don't want to lose the chance. I'm also perfectly OK with keeping the code change here in this PR and making a separate NEWS entry. Please advise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants