Skip to content

gh-120010: fix invalid (nan+nanj) results in _Py_c_prod() #120287

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Dec 6, 2024
Merged

gh-120010: fix invalid (nan+nanj) results in _Py_c_prod() #120287

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
399e3e3
gh-120010: fix invalid (nan+nanj) results in _Py_c_prod()
skirpichev Jun 4, 2024
324c602
address review: declarations
skirpichev Jun 10, 2024
fcafe33
Merge branch 'main' into cc-in-py_c_prod-120010
skirpichev Sep 27, 2024
5509a2d
address review: use subTest
skirpichev Sep 27, 2024
0e98116
Update Misc/NEWS.d/next/Core and Builtins/2024-06-04-08-26-25.gh-issu…
skirpichev Sep 27, 2024
338176d
+ fix typo
skirpichev Sep 27, 2024
83456ef
Merge branch 'main' into cc-in-py_c_prod-120010
skirpichev Nov 27, 2024
219c0cb
+ rename
skirpichev Nov 27, 2024
e368f23
Update Objects/complexobject.c
skirpichev Dec 6, 2024
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
42 changes: 42 additions & 0 deletions Lib/test/test_complex.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,10 @@ def assertFloatsAreIdentical(self, x, y):
msg += ': zeros have different signs'
self.fail(msg.format(x, y))

def assertComplexesAreIdentical(self, x, y):
self.assertFloatsAreIdentical(x.real, y.real)
self.assertFloatsAreIdentical(x.imag, y.imag)

def assertClose(self, x, y, eps=1e-9):
"""Return true iff complexes x and y "are close"."""
self.assertCloseAbs(x.real, y.real, eps)
Expand Down Expand Up @@ -227,6 +231,43 @@ def test_mul(self):
self.assertRaises(TypeError, operator.mul, 1j, None)
self.assertRaises(TypeError, operator.mul, None, 1j)

self.assertComplexesAreIdentical((1e300+1j) * complex(INF, INF),
complex(NAN, INF))
self.assertComplexesAreIdentical(complex(INF, INF) * (1e300+1j),
complex(NAN, INF))
self.assertComplexesAreIdentical((1e300+1j) * complex(NAN, INF),
complex(-INF, INF))
self.assertComplexesAreIdentical(complex(NAN, INF) * (1e300+1j),
complex(-INF, INF))
self.assertComplexesAreIdentical((1e300+1j) * complex(INF, NAN),
complex(INF, INF))
self.assertComplexesAreIdentical(complex(INF, NAN) * (1e300+1j),
complex(INF, INF))
self.assertComplexesAreIdentical(complex(INF, 1) * complex(NAN, INF),
complex(NAN, INF))
self.assertComplexesAreIdentical(complex(INF, 1) * complex(INF, NAN),
complex(INF, NAN))
self.assertComplexesAreIdentical(complex(NAN, INF) * complex(INF, 1),
complex(NAN, INF))
self.assertComplexesAreIdentical(complex(INF, NAN) * complex(INF, 1),
complex(INF, NAN))
self.assertComplexesAreIdentical(complex(NAN, 1) * complex(1, INF),
complex(-INF, NAN))
self.assertComplexesAreIdentical(complex(1, NAN) * complex(1, INF),
complex(NAN, INF))

self.assertComplexesAreIdentical(complex(1e200, NAN) * complex(1e200, NAN),
complex(INF, NAN))
self.assertComplexesAreIdentical(complex(1e200, NAN) * complex(NAN, 1e200),
complex(NAN, INF))
self.assertComplexesAreIdentical(complex(NAN, 1e200) * complex(1e200, NAN),
complex(NAN, INF))
self.assertComplexesAreIdentical(complex(NAN, 1e200) * complex(NAN, 1e200),
complex(-INF, NAN))

self.assertComplexesAreIdentical(complex(NAN, NAN) * complex(NAN, NAN),
complex(NAN, NAN))

def test_mod(self):
# % is no longer supported on complex numbers
with self.assertRaises(TypeError):
Expand Down Expand Up @@ -268,6 +309,7 @@ def test_pow(self):
self.assertAlmostEqual(pow(1j, 200), 1)
self.assertRaises(ValueError, pow, 1+1j, 1+1j, 1+1j)
self.assertRaises(OverflowError, pow, 1e200+1j, 1e200+1j)
self.assertRaises(OverflowError, pow, 1e200+1j, 5)
self.assertRaises(TypeError, pow, 1j, None)
self.assertRaises(TypeError, pow, None, 1j)
self.assertAlmostEqual(pow(1j, 0.5), 0.7071067811865476+0.7071067811865475j)
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2024-06-04-08-26-25.gh-issue-120010._z-AWz.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Correct invalid corner cases (resulted in ``(nan+nanj)`` output) in complex
multiplication, e.g. ``(1e300+1j)*(nan+infj)``. Patch by Sergey B Kirpichev.
61 changes: 58 additions & 3 deletions Objects/complexobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,66 @@ _Py_c_neg(Py_complex a)
}

Py_complex
_Py_c_prod(Py_complex a, Py_complex b)
_Py_c_prod(Py_complex z, Py_complex w)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be ok now. This doesn't require a mixed-mode variant.

Yet, maybe I should change variable names back for consistency with the rest? Current naming here follows to the C standard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@serhiy-storchaka what do you think about renaming of arguments?

I think mapping local variables "abcd" -> "tuvw" will hot hurt readability and then we can preserve original argument names.

Copy link
Member

Choose a reason for hiding this comment

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

I am fine with both variants. Preserving names from the old code can make the diff smaller, but in this case the new code is too different from the old code. Using names from the C standard examples helps comparing our code with the original code. The latter looks more important here.

{
Py_complex r;
r.real = a.real*b.real - a.imag*b.imag;
r.imag = a.real*b.imag + a.imag*b.real;
double a = z.real, b = z.imag, c = w.real, d = w.imag;
double ac = a*c, bd = b*d, ad = a*d, bc = b*c;

r.real = ac - bd;
r.imag = ad + bc;

/* Recover infinities that computed as nan+nanj. See e.g. the C11,
Annex G.5.2, routine _Cmultd(). */
if (isnan(r.real) && isnan(r.imag)) {
int recalc = 0;

if (isinf(a) || isinf(b)) { /* z is infinite */
/* "Box" the infinity and change nans in the other factor to 0 */
a = copysign(isinf(a) ? 1.0 : 0.0, a);
b = copysign(isinf(b) ? 1.0 : 0.0, b);
if (isnan(c)) {
c = copysign(0.0, c);
}
if (isnan(d)) {
d = copysign(0.0, d);
}
recalc = 1;
}
if (isinf(c) || isinf(d)) { /* w is infinite */
/* "Box" the infinity and change nans in the other factor to 0 */
c = copysign(isinf(c) ? 1.0 : 0.0, c);
d = copysign(isinf(d) ? 1.0 : 0.0, d);
if (isnan(a)) {
a = copysign(0.0, a);
}
if (isnan(b)) {
b = copysign(0.0, b);
}
recalc = 1;
}
if (!recalc && (isinf(ac) || isinf(bd) || isinf(ad) || isinf(bc))) {
/* Recover infinities from overflow by changing nans to 0 */
if (isnan(a)) {
a = copysign(0.0, a);
}
if (isnan(b)) {
b = copysign(0.0, b);
}
if (isnan(c)) {
c = copysign(0.0, c);
}
if (isnan(d)) {
d = copysign(0.0, d);
}
recalc = 1;
}
if (recalc) {
r.real = Py_INFINITY*(a*c - b*d);
r.imag = Py_INFINITY*(a*d + b*c);
}
}

return r;
}

Expand Down