Commons Math
  1. Commons Math
  2. MATH-1118

Complex: semantics of equals != Double equals, mismatch with hashCode

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.2
    • Fix Version/s: 3.3
    • Labels:
      None
    • Environment:

      Mac OS 10.9, Java 6, 7

      Description

      Two complex numbers with real/imaginary parts 0.0d but different signs compare as equal numbers. This is according to their mathematical value; the comparison is done via

      return (real == c.real) && (imaginary == c.imaginary);

      Unfortunately, two Double values do NOT compare as equal in that case, so real.equals(c.real) would return false if the signs differ.

      This becomes a problem because for the hashCode, MathUtils.hash is used on the real and imaginary parts, which in turn uses Double.hash.

      This violates the contract on equals/hashCode, so Complex numbers cannot be used in a hashtable or similar data structure:

      Complex c1 = new Complex(0.0, 0.0);
      Complex c2 = new Complex(0.0, -0.0);
      // Checks the contract: equals-hashcode on c1 and c2
      assertTrue("Contract failed: equals-hashcode on c1 and c2", c1.equals(c2) ? c1.hashCode() == c2.hashCode() : true);

      1. Report5a.java
        0.5 kB
        Cyrille Artho
      2. Report5.java
        0.4 kB
        Cyrille Artho
      3. MATH-1118.patch
        4 kB
        Gilles
      4. MATH-1118.patch
        10 kB
        Gilles

        Activity

        Hide
        Cyrille Artho added a comment -

        Dfp instances are affected in the same way (-0.0 == 0.0 but their hash code differs).

        Show
        Cyrille Artho added a comment - Dfp instances are affected in the same way (-0.0 == 0.0 but their hash code differs).
        Hide
        Gilles added a comment -

        When introducing the "correct" (according to JDK for use with "Map") semantics, other unit test fails, especially one that was introduced to ensure the other semantics: MATH-221

        Could you please raise this issue on the "dev" ML? Thanks.

        Show
        Gilles added a comment - When introducing the "correct" (according to JDK for use with "Map") semantics, other unit test fails, especially one that was introduced to ensure the other semantics: MATH-221 Could you please raise this issue on the "dev" ML? Thanks.
        Hide
        Gilles added a comment -

        Patch: proposed change (with one failing test).

        Show
        Gilles added a comment - Patch: proposed change (with one failing test).
        Hide
        Cyrille Artho added a comment -

        It is indeed not clear which way this bug should be fixed.
        Another option (assuming the problem only occurs due to the sign with value 0.0) is to override hashCode such that hashCode(-0.0d) returns hashCode(0.0d), and works as is in the other cases.

        Show
        Cyrille Artho added a comment - It is indeed not clear which way this bug should be fixed. Another option (assuming the problem only occurs due to the sign with value 0.0) is to override hashCode such that hashCode(-0.0d) returns hashCode(0.0d), and works as is in the other cases.
        Hide
        Phil Steitz added a comment -

        Given the decision in MATH-221, I am inclined to agree that the better approach is to just fix the hashcode impl.

        Show
        Phil Steitz added a comment - Given the decision in MATH-221 , I am inclined to agree that the better approach is to just fix the hashcode impl.
        Hide
        Cyrille Artho added a comment -

        I've posted a message on the mailing list. While writing that message, it occurred to me that unnormalized floating point values may break the idea of overriding hashCode for special cases, as there are too many possible cases of unnormalized floats with equal values but different internal representations.
        Therefore, I think Gilles' suggestion is the safest, even though people expecting a mathematical comparison in equals() may be surprised. The update should make it clear in the Javadoc that for a mathematical comparison, "==" must be used.

        Show
        Cyrille Artho added a comment - I've posted a message on the mailing list. While writing that message, it occurred to me that unnormalized floating point values may break the idea of overriding hashCode for special cases, as there are too many possible cases of unnormalized floats with equal values but different internal representations. Therefore, I think Gilles' suggestion is the safest, even though people expecting a mathematical comparison in equals() may be surprised. The update should make it clear in the Javadoc that for a mathematical comparison, "==" must be used.
        Hide
        Cyrille Artho added a comment -

        It seems my previous message overlapped with Phil's. A fix in hashCode would be the most intuitive behavior, but what about unnormalized floats? Does anyone know how that works in Java?
        For example, can we get different representations of two Double numbers with the same value? For example, 1 * 2^1 or 2 * 2^0, which are both 2? In that case, fixing hashCode is hopeless.

        Show
        Cyrille Artho added a comment - It seems my previous message overlapped with Phil's. A fix in hashCode would be the most intuitive behavior, but what about unnormalized floats? Does anyone know how that works in Java? For example, can we get different representations of two Double numbers with the same value? For example, 1 * 2^1 or 2 * 2^0, which are both 2? In that case, fixing hashCode is hopeless.
        Hide
        Gilles added a comment -

        In revision 1587548, I've added the utility method "equals(double, double)" in "MathUtils".

        Show
        Gilles added a comment - In revision 1587548, I've added the utility method "equals(double, double)" in "MathUtils".
        Hide
        Gilles added a comment - - edited

        Uploaded patch for making "equals" compatible with "hashCode" and adding new "equals" methods for testing floating-point equality (using implementations in "o.a.c.m.util.Precision").

        OK to commit?

        Show
        Gilles added a comment - - edited Uploaded patch for making "equals" compatible with "hashCode" and adding new "equals" methods for testing floating-point equality (using implementations in "o.a.c.m.util.Precision"). OK to commit?
        Hide
        Gilles added a comment -

        Committed in revision 1588500.
        Thanks for the report.

        Show
        Gilles added a comment - Committed in revision 1588500. Thanks for the report.
        Hide
        Luc Maisonobe added a comment -

        Closing all resolved issue now available in released 3.3 version.

        Show
        Luc Maisonobe added a comment - Closing all resolved issue now available in released 3.3 version.

          People

          • Assignee:
            Unassigned
            Reporter:
            Cyrille Artho
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development