You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "stefanvodita (via GitHub)" <gi...@apache.org> on 2024/09/05 16:47:11 UTC
[PR] Add unit-of-least-precision float comparison [lucene]
stefanvodita opened a new pull request, #13723:
URL: https://github.com/apache/lucene/pull/13723
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add unit-of-least-precision float comparison [lucene]
Posted by "aherbert (via GitHub)" <gi...@apache.org>.
aherbert commented on code in PR #13723:
URL: https://github.com/apache/lucene/pull/13723#discussion_r1746618586
##########
lucene/test-framework/src/java/org/apache/lucene/tests/util/LuceneTestCase.java:
##########
@@ -881,27 +881,30 @@ public static void assumeNoException(String msg, Exception e) {
* @param maxUlps {@code (maxUlps - 1)} is the number of floating point values between {@code x}
* and {@code y}.
*/
- public static void assertUlpEquals(final float x, final float y, final int maxUlps) {
- assertFalse(Float.isNaN(x));
- assertFalse(Float.isNaN(y));
-
+ public static void assertUlpEquals(final float x, final float y, final short maxUlps) {
final int xInt = Float.floatToRawIntBits(x);
final int yInt = Float.floatToRawIntBits(y);
- final boolean isEqual;
if ((xInt ^ yInt) < 0) {
// Numbers have opposite signs, take care of overflow.
// Remove the sign bit to obtain the absolute ULP above zero.
final int deltaPlus = xInt & Integer.MAX_VALUE;
final int deltaMinus = yInt & Integer.MAX_VALUE;
- // Avoid possible overflow from adding the deltas by using a long.
- isEqual = (long) deltaPlus + deltaMinus <= maxUlps;
- } else {
- // Numbers have same sign, there is no risk of overflow.
- isEqual = Math.abs(xInt - yInt) <= maxUlps;
+ // Note:
+ // If either value is NaN, the exponent bits are set to (255 << 23) and the
+ // distance above 0.0 is always above a short ULP error. So omit the test
+ // for NaN and return directly.
+
+ // Avoid possible overflow from adding the deltas by splitting the comparison
+ assertTrue(deltaPlus <= maxUlps);
+ assertTrue(deltaMinus <= (maxUlps - deltaPlus));
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add unit-of-least-precision float comparison [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #13723:
URL: https://github.com/apache/lucene/pull/13723#issuecomment-2348681854
> > > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add unit-of-least-precision float comparison [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on PR #13723:
URL: https://github.com/apache/lucene/pull/13723#issuecomment-2348502055
> > 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`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add unit-of-least-precision float comparison [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #13723:
URL: https://github.com/apache/lucene/pull/13723#discussion_r1746885657
##########
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);
Review Comment:
is 2 ulps correct here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add unit-of-least-precision float comparison [lucene]
Posted by "stefanvodita (via GitHub)" <gi...@apache.org>.
stefanvodita commented on code in PR #13723:
URL: https://github.com/apache/lucene/pull/13723#discussion_r1746776010
##########
lucene/test-framework/src/java/org/apache/lucene/tests/util/LuceneTestCase.java:
##########
@@ -881,27 +881,30 @@ public static void assumeNoException(String msg, Exception e) {
* @param maxUlps {@code (maxUlps - 1)} is the number of floating point values between {@code x}
* and {@code y}.
*/
- public static void assertUlpEquals(final float x, final float y, final int maxUlps) {
- assertFalse(Float.isNaN(x));
- assertFalse(Float.isNaN(y));
-
+ public static void assertUlpEquals(final float x, final float y, final short maxUlps) {
final int xInt = Float.floatToRawIntBits(x);
final int yInt = Float.floatToRawIntBits(y);
- final boolean isEqual;
if ((xInt ^ yInt) < 0) {
// Numbers have opposite signs, take care of overflow.
// Remove the sign bit to obtain the absolute ULP above zero.
final int deltaPlus = xInt & Integer.MAX_VALUE;
final int deltaMinus = yInt & Integer.MAX_VALUE;
- // Avoid possible overflow from adding the deltas by using a long.
- isEqual = (long) deltaPlus + deltaMinus <= maxUlps;
- } else {
- // Numbers have same sign, there is no risk of overflow.
- isEqual = Math.abs(xInt - yInt) <= maxUlps;
+ // Note:
+ // If either value is NaN, the exponent bits are set to (255 << 23) and the
+ // distance above 0.0 is always above a short ULP error. So omit the test
+ // for NaN and return directly.
+
+ // Avoid possible overflow from adding the deltas by splitting the comparison
+ assertTrue(deltaPlus <= maxUlps);
+ assertTrue(deltaMinus <= (maxUlps - deltaPlus));
Review Comment:
Of course - my mistake! Thanks for pointing it out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add unit-of-least-precision float comparison [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on code in PR #13723:
URL: https://github.com/apache/lucene/pull/13723#discussion_r1750379189
##########
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
Review Comment:
Remove `because`?
##########
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);
Review Comment:
Good question @uschindler -- now we have the fun problem of figuring out how many ULPs can be lost due to different order of operations ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add unit-of-least-precision float comparison [lucene]
Posted by "stefanvodita (via GitHub)" <gi...@apache.org>.
stefanvodita commented on PR #13723:
URL: https://github.com/apache/lucene/pull/13723#issuecomment-2347389307
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add unit-of-least-precision float comparison [lucene]
Posted by "stefanvodita (via GitHub)" <gi...@apache.org>.
stefanvodita commented on code in PR #13723:
URL: https://github.com/apache/lucene/pull/13723#discussion_r1759505470
##########
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
+ summation not being commutative in Java's IEEE 754 implementation. (Alex Herbert, Stefan Vodita)
Review Comment:
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.
##########
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));
Review Comment:
Good point, will do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add unit-of-least-precision float comparison [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on PR #13723:
URL: https://github.com/apache/lucene/pull/13723#issuecomment-2338279298
> Should we add the "double" variant, too?
+1, maybe as follow-on.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add unit-of-least-precision float comparison [lucene]
Posted by "stefanvodita (via GitHub)" <gi...@apache.org>.
stefanvodita merged PR #13723:
URL: https://github.com/apache/lucene/pull/13723
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add unit-of-least-precision float comparison [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #13723:
URL: https://github.com/apache/lucene/pull/13723#discussion_r1759155851
##########
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));
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add unit-of-least-precision float comparison [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #13723:
URL: https://github.com/apache/lucene/pull/13723#issuecomment-2333736267
Can we have a test for the assertion method?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add unit-of-least-precision float comparison [lucene]
Posted by "stefanvodita (via GitHub)" <gi...@apache.org>.
stefanvodita commented on code in PR #13723:
URL: https://github.com/apache/lucene/pull/13723#discussion_r1754952420
##########
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);
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add unit-of-least-precision float comparison [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on code in PR #13723:
URL: https://github.com/apache/lucene/pull/13723#discussion_r1758531891
##########
lucene/test-framework/src/test/org/apache/lucene/tests/util/TestFloatingPointComparisons.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.tests.util;
+
+public class TestFloatingPointComparisons extends LuceneTestCase {
+ public void test() {
+ // Test adjacent numbers
+ assertFalse(doubleEquals(Double.longBitsToDouble(1L), Double.longBitsToDouble(2L), 0));
+ assertTrue(doubleEquals(Double.longBitsToDouble(1L), Double.longBitsToDouble(2L), 1));
+ assertFalse(floatEquals(Float.intBitsToFloat(1), Float.intBitsToFloat(2), (short) 0));
+ assertTrue(floatEquals(Float.intBitsToFloat(1), Float.intBitsToFloat(2), (short) 1));
+
+ // Test signed zeros
+ assertTrue(doubleEquals(0.0d, -0.0d, 0));
+ assertTrue(floatEquals(0.0f, -0.0f, (short) 0));
+
+ // Test NaNs
+ assertFalse(doubleEquals(Double.NaN, Double.NaN, Integer.MAX_VALUE));
+ assertFalse(floatEquals(Float.NaN, Float.NaN, (short) 32767));
Review Comment:
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?
##########
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
Review Comment:
Maybe add `IEEE 754 float summation implemented by Java not being commutative` or so?
Mathematically float summation is fine :)
##########
lucene/test-framework/src/test/org/apache/lucene/tests/util/TestFloatingPointComparisons.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.tests.util;
+
+public class TestFloatingPointComparisons extends LuceneTestCase {
+ public void test() {
+ // Test adjacent numbers
+ assertFalse(doubleEquals(Double.longBitsToDouble(1L), Double.longBitsToDouble(2L), 0));
+ assertTrue(doubleEquals(Double.longBitsToDouble(1L), Double.longBitsToDouble(2L), 1));
+ assertFalse(floatEquals(Float.intBitsToFloat(1), Float.intBitsToFloat(2), (short) 0));
+ assertTrue(floatEquals(Float.intBitsToFloat(1), Float.intBitsToFloat(2), (short) 1));
+
+ // Test signed zeros
+ assertTrue(doubleEquals(0.0d, -0.0d, 0));
+ assertTrue(floatEquals(0.0f, -0.0f, (short) 0));
Review Comment:
Fun.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add unit-of-least-precision float comparison [lucene]
Posted by "stefanvodita (via GitHub)" <gi...@apache.org>.
stefanvodita commented on PR #13723:
URL: https://github.com/apache/lucene/pull/13723#issuecomment-2346657253
Thank you for the feedback! I've added a comparison method for doubles and a test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add unit-of-least-precision float comparison [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #13723:
URL: https://github.com/apache/lucene/pull/13723#issuecomment-2333726460
Should we add the "double" variant, too?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add unit-of-least-precision float comparison [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #13723:
URL: https://github.com/apache/lucene/pull/13723#issuecomment-2346992406
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add unit-of-least-precision float comparison [lucene]
Posted by "stefanvodita (via GitHub)" <gi...@apache.org>.
stefanvodita commented on PR #13723:
URL: https://github.com/apache/lucene/pull/13723#issuecomment-2349071396
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add unit-of-least-precision float comparison [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #13723:
URL: https://github.com/apache/lucene/pull/13723#discussion_r1759153816
##########
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
+ summation not being commutative in Java's IEEE 754 implementation. (Alex Herbert, Stefan Vodita)
Review Comment:
It is always not commutative regardless of implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add unit-of-least-precision float comparison [lucene]
Posted by "stefanvodita (via GitHub)" <gi...@apache.org>.
stefanvodita commented on PR #13723:
URL: https://github.com/apache/lucene/pull/13723#issuecomment-2350917639
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add unit-of-least-precision float comparison [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #13723:
URL: https://github.com/apache/lucene/pull/13723#issuecomment-2350956051
👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org