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

testHypot in test_match uses assertEqual on floats #124040

Open
mcepl opened this issue Sep 13, 2024 · 9 comments
Open

testHypot in test_match uses assertEqual on floats #124040

mcepl opened this issue Sep 13, 2024 · 9 comments
Labels
tests Tests in the Lib/test dir

Comments

@mcepl
Copy link
Contributor

mcepl commented Sep 13, 2024

https://github.com/python/cpython/blob/main/Lib/test/test_math.py#L794-L871

We (SUSE) have test self.assertEqual(hypot(1, -1), math.sqrt(2)) failing on some distros and some architectures (namely i586) and I suspect that whole idea of assertEqual on two floating point numbers is very unfortunate. Shouldn't be there at least assertAlmostEqual?

Cc: @mdickinson, @rhettinger, @danigm

Linked PRs

@Eclips4
Copy link
Member

Eclips4 commented Sep 13, 2024

cc @skirpichev as the author of c61de45

@skirpichev
Copy link
Member

skirpichev commented Sep 13, 2024

IIRC, this test error condition for PyLong_AsDouble/PyFloat_AsDouble (-1 is returned without exception set). I think that we could just drop 1st argument.

I'll double check that & propose a patch.

Edit: using assertAlmostEqual seems ok for me, but most tests in the testHypot() use carefully chosen arguments to compare computed values exactly.

@picnixz picnixz added the tests Tests in the Lib/test dir label Sep 13, 2024
skirpichev added a commit to skirpichev/cpython that referenced this issue Sep 13, 2024
One-argument form is enough to test L2636 and compare computed
values exactly.
skirpichev added a commit to skirpichev/cpython that referenced this issue Sep 13, 2024
One-argument form is enough to test L2636 and compare computed
values exactly.
@skirpichev
Copy link
Member

pr is ready for review: #124042

@danigm
Copy link

danigm commented Sep 13, 2024

There's another similar tests that fails with the same error:

FAIL: testDist (test.test_math.MathTests.testDist)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/abuild/rpmbuild/BUILD/Python-3.13.0rc2/Lib/test/test_math.py", line 972, in testDist
    self.assertEqual(dist((True, True, False, True, False),
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                          (True, False, True, True, False)),
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                     sqrt(2.0))
                     ^^^^^^^^^^
AssertionError: 1.414213562373095 != 1.4142135623730951

@skirpichev
Copy link
Member

Ah, that's from 9c18b1a. I think we could just add two units:

diff --git a/Lib/test/test_math.py b/Lib/test/test_math.py
index c93876e701..dba017a9bb 100644
--- a/Lib/test/test_math.py
+++ b/Lib/test/test_math.py
@@ -969,9 +969,9 @@ def testDist(self):
         self.assertEqual(dist((D(14), D(1)), (D(2), D(-4))), D(13))
         self.assertEqual(dist((F(14, 32), F(1, 32)), (F(2, 32), F(-4, 32))),
                          F(13, 32))
-        self.assertEqual(dist((True, True, False, True, False),
-                              (True, False, True, True, False)),
-                         sqrt(2.0))
+        self.assertEqual(dist((True, True, False, False, True, True),
+                              (True, False, True, False, False, False)),
+                         2.0)
 
         # Test corner cases
         self.assertEqual(dist((13.25, 12.5, -3.25),

@mdickinson
Copy link
Member

I suspect that whole idea of assertEqual on two floating point numbers is very unfortunate.

I believe that the algorithm that @rhettinger implemented for math.hypot has proven and strong error guarantees (though I don't recall whether it guarantees correct rounding or not), but those error guarantees depend on an assumption of strict IEEE 754 semantics. So in the presence of strict IEEE 754 semantics, it doesn't seem inappropriate to be using assertEqual to check hypot results in cases where the error guarantees imply that we should be getting perfect results.

Note that CPython these days does require that the C double type underlying Python's float uses IEEE 754 binary64 format, but that requirement isn't enough to guarantee that the arithmetic operations on doubles follow strict IEEE 754 rules.

At a guess, the problem on i586 is that we don't have strict IEEE 754 semantics, because we're making use of the x87 floating-point instructions (with the 80-bit x87 floating-point format) and so ending up with double rounding on some arithmetic operations.

The other possible cause here is that the code is using FMA/FMS instructions where it shouldn't be (or conversely, is not using FMA/FMS instructions where it's expected to). But my bet's on x87 arithmetic and double rounding.

Possibly we should find a way to detect the x87/double-rounding situation and either skip or weaken the precise tests on machines that have that issue. There's precedent for this elsewhere in the Python test suite (e.g., in the tests for math.fsum, which only guarantees correctly-rounded results in the absence of x87-induced double rounding).

@skirpichev
Copy link
Member

@mcepl, could you add decorator

@unittest.skipIf(HAVE_DOUBLE_ROUNDING, "")

on top of testHypot/testDist and test again?

@mdickinson, thank you for reply.

here's precedent for this elsewhere in the Python test suite

E.g. testHypotAccuracy(). But tests in testHypot/testDist are more basic: sanity checks for arguments, etc. I don't think that filtering out these test functions is a good idea.

BTW, Tim Peters told us, that you are resigned as a core developer (I hope, that my bad pr's weren't at least partially a reason). Yet you are listed in devguide in the expert index. Is it still ok to mention you in relevant topics?

@mdickinson
Copy link
Member

Yet you are listed in devguide in the expert index.

Ah, thanks for pointing that out. I've opened a PR to fix that: python/devguide#1397

Is it still ok to mention you in relevant topics?

Sure, mention away! I may not always have time to respond, though.

@danigm
Copy link

danigm commented Sep 16, 2024

pr is ready for review: #124042

I can confirm that this PR solves all the issues.

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 19, 2024
…y computed results (pythonGH-124042)

(cherry picked from commit 4420cf4)

Co-authored-by: Sergey B Kirpichev <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 19, 2024
…y computed results (pythonGH-124042)

(cherry picked from commit 4420cf4)

Co-authored-by: Sergey B Kirpichev <[email protected]>
serhiy-storchaka pushed a commit that referenced this issue Sep 19, 2024
…ly computed results (GH-124042) (GH-124236)

(cherry picked from commit 4420cf4)

Co-authored-by: Sergey B Kirpichev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Tests in the Lib/test dir
Projects
None yet
Development

No branches or pull requests

6 participants