Add support for NaN and Infinity when comparing matrices for equality.

[GoogleCodeExporter]

What steps will reproduce the problem?
- Run the following code snippet:

        System.out.println(MatrixFeatures.isIdentical(
                new DenseMatrix64F(new double[][] {{ 1.0 }}),
                new DenseMatrix64F(new double[][] {{ Double.NaN }}),
                0.0)
        );

What is the expected output? What do you see instead?
- Observe that this prints "true", while it should actually print "false".

What version of the product are you using? On what operating system?
- EJML 0.15, Windows XP, JDK 1.6.0_21

Original issue reported on code.google.com by kaspar.thommen@gmail.com on 23 Dec 2010 at 7:31

[GoogleCodeExporter]

This is actually a feature of the IEEE 754 floating point standard and not a 
bug in EJML.  Take a look at this webpage to learn more:

http://www.concentric.net/~ttwang/tech/javafloat.htm

I double checked the JavaDoc for isIdentical() and it does correctly describe 
its behavior.  However this is a confusion issue so I have added a comment to 
the JavaDoc about NaN and infinite numbers.  The functions hasNaN() and 
hasUncountable() in MatrixFeatures might be of interest.

Original comment by peter.ab...@gmail.com on 23 Dec 2010 at 1:14

[GoogleCodeExporter]

It is true that any comparison with NaN yields false according to IEEE 754, so 
the condition "if( diff > tol )" is false if "diff" is NaN. But this simply 
means that the method is probably not implemented correctly, because:

a) A call to isIdentical(m1, m2) should yield exactly the same result as 
isIdentical(m1, m2, 0.0) for all matrices m1 and m2, but this is currently 
violated
b) I cannot imagine anyone wanting isIdentical(matrixWithNaSsOnly, 
matrixWithoutNaNs, 0.0) to return true

Note that JUnit's assertEquals(double a, double b, double tol) is implemented 
as follows:

    if (Double.compare(expected, actual) == 0)
        return;
    if (!(Math.abs(expected - actual) <= delta))
        failNotEquals(message, new Double(expected), new Double(actual));

MatrixFeatures.isIdential only implements the analog of the second condition, 
which, IMHO, returns an incorrect result if any of the two values in the 
comparisons is NaN.

Original comment by kaspar.thommen@gmail.com on 29 Dec 2010 at 3:48

[GoogleCodeExporter]

Actually, the link you posted points out this problem in the section "Danger of 
Floating-Point Number Optimization":

! (a < b) does not imply that (a >= b).

! (1.0 < NaN)  ->  true
(1.0 >= NaN)  ->  false

Original comment by kaspar.thommen@gmail.com on 29 Dec 2010 at 3:54

[GoogleCodeExporter]

I do think those are good points and the result is counter intuitive.  Maybe 
have isIdentical() do a more rigorous check similar to what JUnit does and 
create a new function isEquals() that will only produce good results if all the 
numbers are countable.  The reason for two different functions is that I 
suspect that isEquals() would be much faster than isIdentical().

Original comment by peter.ab...@gmail.com on 30 Dec 2010 at 9:13

[GoogleCodeExporter]

I think the only thing that needs to be done in order to fix isIdentical(m1, 
m2, tol) is to replace line 267

        if( diff > tol ) {

by

        if (!(diff <= tol)) {

This change will ensure that non-NaN diff values won't change the condition, 
but if diff is NaN, the condition will be true instead of false now, so the 
method would return false, which makes more sense IMHO.

However, it would also mean that two matrices with all NaNs would NOT be 
considered equal, which is in accordance with the '==' operator on doubles, but 
different from JUnit's behavior and the behavior of Double.equals(). Honestly, 
I'm not sure which one to prefer here...

I'm not sure an additional isEquals() is necessary - when would anyone use 
isEquals() instead of isIdentical(), and for what cases would they produce 
different results?

Original comment by kaspar.thommen@gmail.com on 3 Jan 2011 at 11:23

[GoogleCodeExporter]

Original comment by peter.ab...@gmail.com on 5 Jan 2011 at 5:59

• Changed title: Add support for NaN and Infinity when comparing matrices for equality.
• Changed state: Accepted
• Added labels: Type-Enhancement
• Removed labels: Type-Defect

[GoogleCodeExporter]

MatrixFeatures now contains isEquals and isIdentical.  isEquals performs the 
standard equality test but is designed to always return false if any of the 
elements are NaN or infinite.  isIdentical() explicitly handles uncountable 
numbers and checks to see if they are the same symbol.  See SVN version 249 for 
this change.

The justification for isEquals is that often a matrix having an uncountable 
number is consider a failure condition.  Instead of explicitly checking for NaN 
or infinity and equality, you can just check for equality.

isIdentical() handles the situation where one wants to make sure each element 
is equal to the other or if an element is NaN/Infinite both elements have the 
same symbol. 

Sound good?

Original comment by peter.ab...@gmail.com on 12 Jan 2011 at 1:58

[GoogleCodeExporter]

Hi Peter,

I'm almost happy :-) Why are you considering two infinite numbers as not being 
equal? This contradicts all Java conventions known to me: The '==' operator, 
Double.compare(), Double.equals(), JUnit's assertEquals() all consider two 
positive or two negative infinity values to be equal.

Regards,
Kaspar

Original comment by kaspar.thommen@gmail.com on 14 Jan 2011 at 1:51

[GoogleCodeExporter]

If you call isEquals(a,b) and not isEquals(a,b,tol) (or set tol to zero) then 
you will get what you want.  The reason for this is that infinity minus 
infinity is infinity.  

Original comment by peter.ab...@gmail.com on 14 Jan 2011 at 3:50

[GoogleCodeExporter]

>> "infinity minus infinity is infinity"

I don't think so, try: System.out.println(Double.POSITIVE_INFINITY - 
Double.POSITIVE_INFINITY)

Original comment by kaspar.thommen@gmail.com on 24 Jan 2011 at 7:09

[GoogleCodeExporter]

that's a mistake, infinity minus infinity is NaN.  However the conclusion still 
doesn't change and isEquals(a,b,tol) is following the expected behavior of 
floating point arithmetic.  It's testing if the magnitude of two numbers are 
within tolerance not the == operator.

Original comment by peter.ab...@gmail.com on 24 Jan 2011 at 12:57

[GoogleCodeExporter]

I'm assuming that things are all well and good now so I'm closing this ticket.  
Let me know if this is not the case.

Original comment by peter.ab...@gmail.com on 4 Feb 2011 at 8:59

• Changed state: Fixed