Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions Lib/test/test_cmath.py
Original file line number Diff line number Diff line change
Expand Up @@ -406,8 +406,6 @@ def polar_with_errno_set(z):
_testcapi.set_errno(0)
self.check_polar(polar_with_errno_set)

@unittest.skipIf(sys.platform.startswith("sunos"),
"skipping, see gh-138573")
def test_phase(self):
self.assertAlmostEqual(phase(0), 0.)
self.assertAlmostEqual(phase(1.), 0.)
Expand All @@ -423,6 +421,16 @@ def test_phase(self):
self.assertEqual(phase(complex(-0.0, 0.0)), pi)
self.assertEqual(phase(complex(-0.0, -0.0)), -pi)

# overflow and underflow of imag/real
self.assertEqual(phase(complex(1E300, 1E-320)), 0.0)
self.assertEqual(phase(complex(1E300, -1E-320)), -0.0)
self.assertAlmostEqual(phase(complex(1E-320, 1E300)), pi/2)
self.assertAlmostEqual(phase(complex(-1E-320, 1E300)), pi/2)
self.assertAlmostEqual(phase(complex(-1E300, 1E-320)), pi)
self.assertAlmostEqual(phase(complex(-1E300, -1E-320)), -pi)
self.assertAlmostEqual(phase(complex(1E-320, -1E300)), -pi/2)
self.assertAlmostEqual(phase(complex(-1E-320, -1E300)), -pi/2)

# infinities
self.assertAlmostEqual(phase(complex(-INF, -0.0)), -pi)
self.assertAlmostEqual(phase(complex(-INF, -2.3)), -pi)
Expand Down
2 changes: 0 additions & 2 deletions Lib/test/test_math.py
Original file line number Diff line number Diff line change
Expand Up @@ -287,8 +287,6 @@ def testAtanh(self):
self.assertRaises(ValueError, math.atanh, NINF)
self.assertTrue(math.isnan(math.atanh(NAN)))

@unittest.skipIf(sys.platform.startswith("sunos"),
"skipping, see gh-138573")
def testAtan2(self):
self.assertRaises(TypeError, math.atan2)
self.ftest('atan2(-1, 0)', math.atan2(-1, 0), -math.pi/2)
Expand Down
Original file line number Diff line number Diff line change
@@ -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?

the :func:`cmath.phase` of ``z`` should not raise ``OverflowError``. Also,
given arguments of zero, Python's :func:`math.atan2`, :func:`math.atan2pi`,
and :func:`cmath.phase` should return the values specified by Annex F of the
C standard, not raise ``ValueError``, regardless of whether the platform's
math library sets ``errno`` for ``atan2(0.0, 0.0)``, ``atan2(0.0, -0.0)``, etc.
Contributed by High Performance Kernels LLC.
11 changes: 5 additions & 6 deletions Modules/cmathmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -1004,12 +1004,11 @@ cmath_phase_impl(PyObject *module, Py_complex z)
{
double phi;

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

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.

* EDOM, which we should ignore, or ERANGE if phi underflows,
* which is silent on Python. Overflow is not possible. */
return PyFloat_FromDouble(phi);
}

/*[clinic input]
Expand Down
39 changes: 35 additions & 4 deletions Modules/mathmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -1017,7 +1017,7 @@ math_1a(PyObject *arg, double (*func) (double), const char *err_msg)
The last rule is used to catch overflow on platforms which follow
C89 but for which HUGE_VAL is not an infinity.

For most two-argument functions (copysign, fmod, hypot, atan2)
For most two-argument functions (fmod, hypot)
these rules are enough to ensure that Python's functions behave as
specified in 'Annex F' of the C99 standard, with the 'invalid' and
'divide-by-zero' floating-point exceptions mapping to Python's
Expand Down Expand Up @@ -1060,6 +1060,28 @@ math_2(PyObject *const *args, Py_ssize_t nargs,
return PyFloat_FromDouble(r);
}

/* 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). */

static PyObject *
math_2ne(PyObject *const *args, Py_ssize_t nargs,
double (*func) (double, double), const char *funcname)
{
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.)

x = PyFloat_AsDouble(args[0]);
if (x == -1.0 && PyErr_Occurred()) {
return NULL;
}
y = PyFloat_AsDouble(args[1]);
if (y == -1.0 && PyErr_Occurred()) {
return NULL;
}
r = (*func)(x, y); // Ignore errno on purpose.
Comment thread
hpkfft marked this conversation as resolved.
return PyFloat_FromDouble(r);
}

#define FUNC1(funcname, func, can_overflow, docstring) \
static PyObject * math_##funcname(PyObject *self, PyObject *args) { \
return math_1(args, func, can_overflow, NULL); \
Expand Down Expand Up @@ -1090,6 +1112,12 @@ math_2(PyObject *const *args, Py_ssize_t nargs,
}\
PyDoc_STRVAR(math_##funcname##_doc, docstring);

#define FUNC2NE(funcname, func, docstring) \
static PyObject * math_##funcname(PyObject *self, PyObject *const *args, Py_ssize_t nargs) { \
return math_2ne(args, nargs, func, #funcname); \
}\
PyDoc_STRVAR(math_##funcname##_doc, docstring);

FUNC1D(acos, acos, 0,
"acos($module, x, /)\n--\n\n"
"Return the arc cosine (measured in radians) of x.\n\n"
Expand Down Expand Up @@ -1121,11 +1149,14 @@ FUNC1(atan, atan, 0,
"atan($module, x, /)\n--\n\n"
"Return the arc tangent (measured in radians) of x.\n\n"
"The result is between -pi/2 and pi/2.")
FUNC2(atan2, atan2,
/* gh-153144: Ignore atan2 and atan2pi errno on purpose since it can optionally
* be EDOM, which we should ignore, or ERANGE on underflow, which is
* silent on Python. Overflow is not possible. */
FUNC2NE(atan2, atan2,
"atan2($module, y, x, /)\n--\n\n"
"Return the arc tangent (measured in radians) of y/x.\n\n"
"Unlike atan(y/x), the signs of both x and y are considered.")
FUNC2(atan2pi, m_atan2pi,
FUNC2NE(atan2pi, m_atan2pi,
"atan2pi($module, y, x, /)\n--\n\n"
"Return the arc tangent (measured in half-turns) of y/x.\n\n"
"Unlike atanpi(y/x), the signs of both x and y are considered.")
Expand Down Expand Up @@ -1178,7 +1209,7 @@ math_ceil(PyObject *module, PyObject *number)
return PyLong_FromDouble(ceil(x));
}

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.

"copysign($module, x, y, /)\n--\n\n"
"Return a float with the magnitude (absolute value) of x but the sign of y.\n\n"
"On platforms that support signed zeros, copysign(1.0, -0.0)\n"
Expand Down
Loading