Commons Math
  1. Commons Math
  2. MATH-1118

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

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.2
    • Fix Version/s: 4.0, 3.6
    • 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. Report5.java
        0.4 kB
        Cyrille Artho
      2. Report5a.java
        0.5 kB
        Cyrille Artho
      3. MATH-1118.patch
        4 kB
        Gilles
      4. MATH-1118.patch
        10 kB
        Gilles

        Activity

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        4d 9h 56m 1 Gilles 18/Apr/14 17:00
        Resolved Resolved Closed Closed
        30d 23h 12m 1 Luc Maisonobe 19/May/14 16:13
        Closed Closed Reopened Reopened
        352d 15h 21m 1 Cyrille Artho 07/May/15 07:35
        Reopened Reopened Resolved Resolved
        8h 12m 1 Luc Maisonobe 07/May/15 15:47
        Luc Maisonobe made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Fix Version/s 4.0 [ 12317577 ]
        Fix Version/s 3.6 [ 12332157 ]
        Fix Version/s 3.3 [ 12324600 ]
        Resolution Fixed [ 1 ]
        Hide
        Luc Maisonobe added a comment -

        Fixed in git repository on both the master branch and the 3.X branch.

        Thanks for the reminding.

        Show
        Luc Maisonobe added a comment - Fixed in git repository on both the master branch and the 3.X branch. Thanks for the reminding.
        Cyrille Artho made changes -
        Resolution Fixed [ 1 ]
        Status Closed [ 6 ] Reopened [ 4 ]
        Hide
        Cyrille Artho added a comment -

        The bug has been fixed for class Complex, but not for Dfp. The code in "Report5a.java" still fails. The nature of the problem is exactly the same as in "Report5.java", so I have reported the bug in the same place.

        If you would prefer a new issue on this instead, I can file "Report5a.java" as a new bug.

        Show
        Cyrille Artho added a comment - The bug has been fixed for class Complex, but not for Dfp. The code in "Report5a.java" still fails. The nature of the problem is exactly the same as in "Report5.java", so I have reported the bug in the same place. If you would prefer a new issue on this instead, I can file "Report5a.java" as a new bug.
        Luc Maisonobe made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        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.
        Gilles made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 3.3 [ 12324600 ]
        Resolution Fixed [ 1 ]
        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.
        Gilles made changes -
        Attachment MATH-1118.patch [ 12640615 ]
        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 -

        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
        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
        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
        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 -

        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.
        Gilles made changes -
        Attachment MATH-1118.patch [ 12640066 ]
        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
        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.
        Cyrille Artho made changes -
        Attachment Report5a.java [ 12640036 ]
        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).
        Cyrille Artho made changes -
        Field Original Value New Value
        Attachment Report5.java [ 12640028 ]
        Cyrille Artho created issue -

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development