You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "iverase (via GitHub)" <gi...@apache.org> on 2025/03/28 10:14:21 UTC
[PR] Handle NaN results in TestVectorUtilSupport.testBinaryVectors [lucene]
iverase opened a new pull request, #14419:
URL: https://github.com/apache/lucene/pull/14419
I notice the following error in CI:
```
./gradlew test --tests TestVectorUtilSupport.testBinaryVectors -Dtests.seed=B12D50704230E803 -Dtests.locale=ce -Dtests.timezone=SystemV/AST4 -Dtests.asserts=true -Dtests.file.encoding=UTF-8
> java.lang.AssertionError:
> Expected: a numeric value within <1.0E-5> of <NaN>
> but: <NaN> differed by <NaN> more than delta <1.0E-5>
> at __randomizedtesting.SeedInfo.seed([B12D50704230E803:FF2F6188C28072F0]:0)
> at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
> at org.junit.Assert.assertThat(Assert.java:964)
> at org.junit.Assert.assertThat(Assert.java:930)
> at org.apache.lucene.internal.vectorization.TestVectorUtilSupport.assertFloatReturningProviders(TestVectorUtilSupport.java:213)
> at org.apache.lucene.internal.vectorization.TestVectorUtilSupport.testBinaryVectors(TestVectorUtilSupport.java:71)
> at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
```
To avoid this, this commit makes `assertFloatReturningProviders` resilient to NaN results.
--
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] Handle NaN results in TestVectorUtilSupport.testBinaryVectors [lucene]
Posted by "iverase (via GitHub)" <gi...@apache.org>.
iverase merged PR #14419:
URL: https://github.com/apache/lucene/pull/14419
--
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] Handle NaN results in TestVectorUtilSupport.testBinaryVectors [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on code in PR #14419:
URL: https://github.com/apache/lucene/pull/14419#discussion_r2018614545
##########
lucene/core/src/test/org/apache/lucene/internal/vectorization/TestVectorUtilSupport.java:
##########
@@ -210,9 +210,13 @@ public void testMinMaxScalarQuantize() {
}
private void assertFloatReturningProviders(ToDoubleFunction<VectorUtilSupport> func) {
- assertThat(
- func.applyAsDouble(PANAMA_PROVIDER.getVectorUtilSupport()),
- closeTo(func.applyAsDouble(LUCENE_PROVIDER.getVectorUtilSupport()), delta));
+ double luceneValue = func.applyAsDouble(LUCENE_PROVIDER.getVectorUtilSupport());
Review Comment:
I think `closeTo` gives better error messages (specifying the delta, as in the error message, rather than just giving the two values and letting you work it out). It's also a clearer assert specification in code.
--
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] Handle NaN results in TestVectorUtilSupport.testBinaryVectors [lucene]
Posted by "gf2121 (via GitHub)" <gi...@apache.org>.
gf2121 commented on code in PR #14419:
URL: https://github.com/apache/lucene/pull/14419#discussion_r2018496277
##########
lucene/core/src/test/org/apache/lucene/internal/vectorization/TestVectorUtilSupport.java:
##########
@@ -210,9 +210,13 @@ public void testMinMaxScalarQuantize() {
}
private void assertFloatReturningProviders(ToDoubleFunction<VectorUtilSupport> func) {
- assertThat(
- func.applyAsDouble(PANAMA_PROVIDER.getVectorUtilSupport()),
- closeTo(func.applyAsDouble(LUCENE_PROVIDER.getVectorUtilSupport()), delta));
+ double luceneValue = func.applyAsDouble(LUCENE_PROVIDER.getVectorUtilSupport());
Review Comment:
I was a bit surprised that we did not have a good utility to deal double equals so I grep `1e-5` in the code base. It seems we used to use `public static void assertEquals(double expected, double actual, double delta)` for this kinda assertion. It deals `NaN` well.
Do we have a special reason to use `closeTo` 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] Handle NaN results in TestVectorUtilSupport.testBinaryVectors [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #14419:
URL: https://github.com/apache/lucene/pull/14419#discussion_r2018509188
##########
lucene/core/src/test/org/apache/lucene/internal/vectorization/TestVectorUtilSupport.java:
##########
@@ -210,9 +210,13 @@ public void testMinMaxScalarQuantize() {
}
private void assertFloatReturningProviders(ToDoubleFunction<VectorUtilSupport> func) {
- assertThat(
- func.applyAsDouble(PANAMA_PROVIDER.getVectorUtilSupport()),
- closeTo(func.applyAsDouble(LUCENE_PROVIDER.getVectorUtilSupport()), delta));
+ double luceneValue = func.applyAsDouble(LUCENE_PROVIDER.getVectorUtilSupport());
Review Comment:
Using `assertEquals` is fine with the delta. I don't know of any special reason to use `closeTo` here. @thecoop what do you think?
--
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] Handle NaN results in TestVectorUtilSupport.testBinaryVectors [lucene]
Posted by "iverase (via GitHub)" <gi...@apache.org>.
iverase commented on code in PR #14419:
URL: https://github.com/apache/lucene/pull/14419#discussion_r2018525082
##########
lucene/core/src/test/org/apache/lucene/internal/vectorization/TestVectorUtilSupport.java:
##########
@@ -210,9 +210,13 @@ public void testMinMaxScalarQuantize() {
}
private void assertFloatReturningProviders(ToDoubleFunction<VectorUtilSupport> func) {
- assertThat(
- func.applyAsDouble(PANAMA_PROVIDER.getVectorUtilSupport()),
- closeTo(func.applyAsDouble(LUCENE_PROVIDER.getVectorUtilSupport()), delta));
+ double luceneValue = func.applyAsDouble(LUCENE_PROVIDER.getVectorUtilSupport());
Review Comment:
I updated the code to use `assertEquals`
--
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] Handle NaN results in TestVectorUtilSupport.testBinaryVectors [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on code in PR #14419:
URL: https://github.com/apache/lucene/pull/14419#discussion_r2018614545
##########
lucene/core/src/test/org/apache/lucene/internal/vectorization/TestVectorUtilSupport.java:
##########
@@ -210,9 +210,13 @@ public void testMinMaxScalarQuantize() {
}
private void assertFloatReturningProviders(ToDoubleFunction<VectorUtilSupport> func) {
- assertThat(
- func.applyAsDouble(PANAMA_PROVIDER.getVectorUtilSupport()),
- closeTo(func.applyAsDouble(LUCENE_PROVIDER.getVectorUtilSupport()), delta));
+ double luceneValue = func.applyAsDouble(LUCENE_PROVIDER.getVectorUtilSupport());
Review Comment:
I think `closeTo` gives better error messages (specifying the delta, as in the error message, rather than just giving the two values and letting you work it out). It's also a clearer assert specification in code.
But yeah, hadn't spotted it doesn't handle NaN in the same way.
--
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