Add unit-of-least-precision float comparison #13723

stefanvodita · 2024-09-05T16:47:08Z

Comparing floats with a fixed epsilon doesn't really work. We add comparison based on unit-of-lest-precision (ULP) and use it to fix a failing test.

Closes #13720

aherbert · 2024-09-06T07:14:39Z

lucene/test-framework/src/java/org/apache/lucene/tests/util/LuceneTestCase.java

+
+      // Avoid possible overflow from adding the deltas by splitting the comparison
+      assertTrue(deltaPlus <= maxUlps);
+      assertTrue(deltaMinus <= (maxUlps - deltaPlus));


You should add a return inside this if conditional. Otherwise you fall through to a condition check that may return an incorrect result if xInt - yInt overflows.

Of course - my mistake! Thanks for pointing it out.

uschindler · 2024-09-06T10:09:29Z

Should we add the "double" variant, too?

uschindler · 2024-09-06T10:13:51Z

lucene/facet/src/test/org/apache/lucene/facet/taxonomy/TestTaxonomyFacetAssociations.java

@@ -654,7 +654,7 @@ private void assertFloatFacetResultsEqual(List<FacetResult> expected, List<Facet

      assertEquals(expectedResult.dim, actualResult.dim);
      assertArrayEquals(expectedResult.path, actualResult.path);
-      assertEquals((float) expectedResult.value, (float) actualResult.value, 2e-1);
+      assertUlpEquals((float) expectedResult.value, (float) actualResult.value, (short) 2);


is 2 ulps correct here?

Good question @uschindler -- now we have the fun problem of figuring out how many ULPs can be lost due to different order of operations ...

It's correct in the sense that it's the smallest value that will pass the test that was failing. Is it the smallest value that will pass this test on any seed? That I'm not sure. I've run 500 iterations and didn't see the test fail again.

uschindler · 2024-09-06T10:14:59Z

Can we have a test for the assertion method?

mikemccand · 2024-09-09T14:28:09Z

Should we add the "double" variant, too?

+1, maybe as follow-on.

mikemccand

Thank you @stefanvodita!

mikemccand · 2024-09-09T14:28:49Z

lucene/CHANGES.txt

@@ -422,7 +422,9 @@ Build

 Other
 --------------------
-(No changes)
+
+* GITHUB#13720: Add float comparison based on unit of least precision and use it to stop test failures because caused by


Remove because?

mikemccand · 2024-09-09T14:31:30Z

lucene/facet/src/test/org/apache/lucene/facet/taxonomy/TestTaxonomyFacetAssociations.java

@@ -654,7 +654,7 @@ private void assertFloatFacetResultsEqual(List<FacetResult> expected, List<Facet

      assertEquals(expectedResult.dim, actualResult.dim);
      assertArrayEquals(expectedResult.path, actualResult.path);
-      assertEquals((float) expectedResult.value, (float) actualResult.value, 2e-1);
+      assertUlpEquals((float) expectedResult.value, (float) actualResult.value, (short) 2);


Good question @uschindler -- now we have the fun problem of figuring out how many ULPs can be lost due to different order of operations ...

stefanvodita · 2024-09-12T15:49:09Z

Thank you for the feedback! I've added a comparison method for doubles and a test.

uschindler · 2024-09-12T18:40:56Z

I don't like the last commit because it changes from a assert-like method to a boolean returning method.

Could we not keep the previous method signature and still add a test?

stefanvodita · 2024-09-12T23:08:26Z

I changed it away from an assertion because I liked this more. It makes it so you can assert on floats not being equal or use their equality in a condition, without making an assertion statement much longer. Do you not like it because it's too generic? We could move it to TestUtil or we could provide assertion methods alongside the equality methods, if that helps.

mikemccand · 2024-09-13T09:39:45Z

I don't like the last commit because it changes from a assert-like method to a boolean returning method.

I changed it away from an assertion because I liked this more. It makes it so you can assert on floats not being equal or use their equality in a condition, without making an assertion statement much longer. Do you not like it because it's too generic? We could move it to TestUtil or we could provide assertion methods alongside the equality methods, if that helps.

Maybe we could do both?

I.e. add the TestUtil boolean-returning methods (float/doubleEquals) to TestUtil but then also add the sugar methods void assertFloat/DoubleUlpEquals?

mikemccand

Thanks @stefanvodita! It's wild how complicated floating point numbers are for computers yet how tantalizingly simple our modern programming languages make them seem.

mikemccand · 2024-09-13T09:28:34Z

lucene/test-framework/src/test/org/apache/lucene/tests/util/TestFloatingPointComparisons.java

+
+    // Test signed zeros
+    assertTrue(doubleEquals(0.0d, -0.0d, 0));
+    assertTrue(floatEquals(0.0f, -0.0f, (short) 0));


mikemccand · 2024-09-13T09:33:53Z

lucene/test-framework/src/test/org/apache/lucene/tests/util/TestFloatingPointComparisons.java

+
+    // Test NaNs
+    assertFalse(doubleEquals(Double.NaN, Double.NaN, Integer.MAX_VALUE));
+    assertFalse(floatEquals(Float.NaN, Float.NaN, (short) 32767));


You could maybe also use Math.nextAfter to add one or two ulps to a random float/double and then assert that float/doubleEquals agrees?

mikemccand · 2024-09-13T09:35:55Z

lucene/CHANGES.txt

@@ -422,7 +422,9 @@ Build

 Other
 --------------------
-(No changes)
+
+* GITHUB#13720: Add float comparison based on unit of least precision and use it to stop test failures caused by float


Maybe add IEEE 754 float summation implemented by Java not being commutative or so?

Mathematically float summation is fine :)

uschindler · 2024-09-13T11:11:56Z

I don't like the last commit because it changes from a assert-like method to a boolean returning method.

I changed it away from an assertion because I liked this more. It makes it so you can assert on floats not being equal or use their equality in a condition, without making an assertion statement much longer. Do you not like it because it's too generic? We could move it to TestUtil or we could provide assertion methods alongside the equality methods, if that helps.

Maybe we could do both?

I.e. add the TestUtil boolean-returning methods (float/doubleEquals) to TestUtil but then also add the sugar methods void assertFloat/DoubleUlpEquals?

That was my intention.

stefanvodita · 2024-09-13T14:17:32Z

I've moved the methods around and, as I was writing more tests, realised I'm not going to be as comprehensive as the originals tests, so I adapted those instead.

mikemccand

Thanks @stefanvodita!

uschindler · 2024-09-13T16:28:11Z

lucene/CHANGES.txt

-(No changes)
+
+* GITHUB#13720: Add float comparison based on unit of least precision and use it to stop test failures caused by float
+  summation not being commutative in Java's IEEE 754 implementation. (Alex Herbert, Stefan Vodita)


It is always not commutative regardless of implementation.

Now that I think about it, it's not even a commutativity issue. It's an associativity issue.
a + b == b + a
But
(a + b) + c != a + (b + c)
I'll fix the entry.

uschindler · 2024-09-13T16:29:43Z

lucene/test-framework/src/java/org/apache/lucene/tests/util/LuceneTestCase.java

@@ -864,6 +864,14 @@ public static void assumeNoException(String msg, Exception e) {
    RandomizedTest.assumeNoException(msg, e);
  }

+  public static void assertFloatUlpEquals(final float x, final float y, final short maxUlps) {
+    assertTrue(TestUtil.floatUlpEquals(x, y, maxUlps));


Maybe add a message showing string of both values.
The problem with assert true is that you get no useful output.
This is why I wanted to have the assert methods.

Good point, will do.

uschindler

Looks fine.

One place where we should add this is the comparison of Panama vector vs. scalar vector test. The multiplication and summations when using Panama are executed in different order (the reason why we need a hand optimized impl). The test uses a quote large epsilon. It is only unclear to ne how many ulps we need for a dot product! Number of dimensions or maybe only number of splits of dimensions?

stefanvodita · 2024-09-14T08:49:26Z

Thank you Mike and Uwe! I opened a separate issue to replace various epsilon-based equality checks (#13789), since that could be a large enough task, and I'll merge this one.

uschindler · 2024-09-14T11:16:07Z

👍

Add unit-of-least-precision float comparison

d1b60aa

aherbert mentioned this pull request Sep 5, 2024

Check for NaN early when doing comparison apache/commons-numbers#142

Closed

Use recommended comparison

21422f8

aherbert reviewed Sep 6, 2024

View reviewed changes

Fix mistaken fall-through case

720be0a

uschindler reviewed Sep 6, 2024

View reviewed changes

This was referenced Sep 6, 2024

Remove leftover search(Query, Collector) usages in TestTaxonomyFacetAssociations #13726

Merged

Improve TestTaxonomyFacetAssociations#validateFloats to not rely on summation ordering #13738

Open

mikemccand approved these changes Sep 9, 2024

View reviewed changes

stefanvodita added 3 commits September 11, 2024 15:15

Add double comparison

deeac90

Fix CHANGES entry

c2cd4be

Test comparison methods

d8e5d76

mikemccand approved these changes Sep 13, 2024

View reviewed changes

stefanvodita added 4 commits September 13, 2024 12:59

Move equality methods to TestUtil

4dcd94e

Improve CHANGES entry

96df370

[WiP] Add more tests

79b866a

Add Commons Numbers tests

af2aabc

mikemccand approved these changes Sep 13, 2024

View reviewed changes

uschindler reviewed Sep 13, 2024

View reviewed changes

Add messages to the new assertions

e65c3da

Fix CHANGES entry

0297cc0

uschindler approved these changes Sep 14, 2024

View reviewed changes

stefanvodita mentioned this pull request Sep 14, 2024

Use ULP float comparison instead of epsilon-based comparison #13789

Open

stefanvodita merged commit aa86a81 into apache:main Sep 14, 2024
3 checks passed

stefanvodita added a commit that referenced this pull request Sep 14, 2024

Add unit-of-least-precision float comparison (#13723)

b7fd00c

stefanvodita added this to the 9.12.0 milestone Sep 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit-of-least-precision float comparison #13723

Add unit-of-least-precision float comparison #13723

stefanvodita commented Sep 5, 2024

aherbert Sep 6, 2024

stefanvodita Sep 6, 2024

uschindler commented Sep 6, 2024

uschindler Sep 6, 2024

mikemccand Sep 9, 2024

stefanvodita Sep 11, 2024

uschindler commented Sep 6, 2024

mikemccand commented Sep 9, 2024

mikemccand left a comment

mikemccand Sep 9, 2024

mikemccand Sep 9, 2024

stefanvodita commented Sep 12, 2024

uschindler commented Sep 12, 2024

stefanvodita commented Sep 12, 2024

mikemccand commented Sep 13, 2024

mikemccand left a comment

mikemccand Sep 13, 2024

mikemccand Sep 13, 2024

mikemccand Sep 13, 2024

uschindler commented Sep 13, 2024

stefanvodita commented Sep 13, 2024

mikemccand left a comment

uschindler Sep 13, 2024

stefanvodita Sep 13, 2024

uschindler Sep 13, 2024

stefanvodita Sep 13, 2024

uschindler left a comment

stefanvodita commented Sep 14, 2024

uschindler commented Sep 14, 2024

Add unit-of-least-precision float comparison #13723

Add unit-of-least-precision float comparison #13723

Conversation

stefanvodita commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uschindler commented Sep 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uschindler commented Sep 6, 2024

mikemccand commented Sep 9, 2024

mikemccand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanvodita commented Sep 12, 2024

uschindler commented Sep 12, 2024

stefanvodita commented Sep 12, 2024

mikemccand commented Sep 13, 2024

mikemccand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uschindler commented Sep 13, 2024

stefanvodita commented Sep 13, 2024

mikemccand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uschindler left a comment

Choose a reason for hiding this comment

stefanvodita commented Sep 14, 2024

uschindler commented Sep 14, 2024