[
https://issues.apache.org/jira/browse/NUMBERS-77?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17329332#comment-17329332
]
Alex Herbert commented on NUMBERS-77:
-------------------------------------
{quote}I think that there is a problem when {{0 < |a| < eps}}
{quote}
Good spot. It will return a and not 0.
Looking at Matt's example in AbstractHyperPlane then this is a case where -0.0
should not return -1 if you test it with 0.0. But that should be handled by an
epsilon for the eqZero call.
In your diff for the sign method your javadoc states:
{noformat}
* The returned value is
* <ul>
* <li>{@code 0} if the arguments are considered equal,</li>
* <li>{@code -1} if {@code a < b},</li>
* <li>{@code +1} if {@code a > b} or if either value is NaN.</li>
* </ul>{noformat}
This is copied from compare. It should be:
{noformat}
* The returned value is
* <ul>
* <li>{@code 0} if the arguments is considered equal to zero,</li>
* <li>{@code -1} if {@code a < 0},</li>
* <li>{@code +1} if {@code a > 0} or if either value is NaN.</li>
* </ul>{noformat}
Thinking about the result for compare, it is documented to return +1 when
either argument is NaN. If this class is ever used for sorting (as part of a
comparison of objects with double values) then this will result in an unstable
sort as this violates the expected behaviour:
{noformat}
if compare(a, NaN) == 1 then compare(NaN, a) == -1
{noformat}
Double.compare will place NaN after any non-Nan value, or any non-NaN value
before a NaN. It returns 0 for two NaN inputs.
{noformat}
compare(a, NaN) == -1
compare(NaN, a) == 1
compare(NaN, NaN) == 0
{noformat}
I think that NaN requires some special handling. For example assuming compare
returns 1 for NaN on either input you have:
{noformat}
eq(a, NaN) == false
eqZero(NaN) == false
lt(a, NaN) == false
lt(NaN, a) == false
lte(a, NaN) == false
lte(NaN, a) == false
gt(a, NaN) == true
gt(NaN, a) == true
gte(a, NaN) == true
gte(NaN, a) == true
sign(NaN) == 1{noformat}
This issue is also present in the Precision class which will return +1 for the
compare methods when NaN is one of the arguments. All the Precision methods for
equality state that nothing is equal to NaN and there are separate methods for
equalsIncludingNan. But the compare method does not specially handle it and so
cannot be used for sorting/ordering.
If you have an object that implements equals and compare using Precision.equals
and Precision.compareTo then it cannot be used in a sort as comparisons with
NaN always return 1. If you use Precision.equalsIncludingNan then the
comparator will fail its contract as two equal objects (NaN, NaN) will not have
zero returned from compare(NaN, NaN).
So currently the comparisons break down when NaN is input. Is this the
behaviour we want? If so it should be documented to not use these methods in
comparators for sorting.
Note you can test this using:
{code:java}
@Test
void testUnstableSort() {
Double[] array = {1.0, 2.0, 3.0, Double.NaN, Double.NaN};
for (int i = 0; i < 10; i++) {
Collections.shuffle(Arrays.asList(array));
Arrays.sort(array, (a, b) -> Precision.compareTo(a, b, 1e-10));
// CHECKSTYLE stop all
System.out.println(Arrays.toString(array));
}
}
{code}
Outputs some rubbish:
{noformat}
[NaN, 1.0, 2.0, 3.0, NaN]
[2.0, NaN, 1.0, NaN, 3.0]
[1.0, 2.0, 3.0, NaN, NaN]
[3.0, NaN, NaN, 1.0, 2.0]
[3.0, NaN, 1.0, 2.0, NaN]
[2.0, NaN, NaN, 1.0, 3.0]
[1.0, NaN, NaN, 2.0, 3.0]
[3.0, NaN, 1.0, NaN, 2.0]
[1.0, NaN, NaN, 2.0, 3.0]
[3.0, NaN, 1.0, 2.0, NaN]
{noformat}
Switching to Double.compare and it works.
> Move utilities from "Commons Geometry"
> --------------------------------------
>
> Key: NUMBERS-77
> URL: https://issues.apache.org/jira/browse/NUMBERS-77
> Project: Commons Numbers
> Issue Type: Task
> Reporter: Gilles Sadowski
> Priority: Major
> Fix For: 1.1
>
> Attachments: NUMBERS-77.diff
>
> Time Spent: 1h 10m
> Remaining Estimate: 0h
>
> "Commons Geometry" defines utilities that would be a better fit in this
> component.
> Duplication of general-purpose codes should be avoided, in order to benefit
> from consolidated usage (bug reporting, performance enhancements, ...).
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
