Commons Math
  1. Commons Math
  2. MATH-471

MathUtils.equals(double, double) does not work properly for floats

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.2, 3.0
    • Labels:
      None

      Description

      MathUtils.equals(double, double) does not work properly for floats.

      There is no equals(float,float) so float parameters are automatically promoted to double. However, that is not necessarily appropriate, given that the ULP for a double is much smaller than the ULP for a float.

      So for example:

      double oneDouble = 1.0d;
      assertTrue(MathUtils.equals(oneDouble, Double.longBitsToDouble(1 + Double.doubleToLongBits(oneDouble)))); // OK
      float oneFloat = 1.0f;
      assertTrue(MathUtils.equals(oneFloat, Float.intBitsToFloat(1 + Float.floatToIntBits(oneFloat)))); // FAILS
      float  f1 = 333.33334f;
      double d1 = 333.33334d;
      assertTrue(MathUtils.equals(d1, f1)); // FAILS
      

      I think the equals() methods need to be duplicated with the appropriate changes for floats to avoid any problems with the promotion of floats.

        Activity

        Hide
        Sebb added a comment -

        Implementation of equals(float...) methods

        Show
        Sebb added a comment - Implementation of equals(float...) methods
        Hide
        Sebb added a comment -

        I now realise that the 3rd example above may not be easy - or even possible - to fix, but it does offer a good example of the difficulties of comparing floating point numbers!

        Show
        Sebb added a comment - I now realise that the 3rd example above may not be easy - or even possible - to fix, but it does offer a good example of the difficulties of comparing floating point numbers!
        Hide
        Phil Steitz added a comment -

        Ouch!

        Thanks for reporting and patching this. Sebb.

        +1 for applying the patch to both 2_X and trunk, with unit tests.

        Show
        Phil Steitz added a comment - Ouch! Thanks for reporting and patching this. Sebb. +1 for applying the patch to both 2_X and trunk, with unit tests.
        Hide
        Phil Steitz added a comment -

        It is not obvious to me that the third example should succeed. An argument could be made that the existing code does the right thing when passed a double and a float, which is to convert the float according to the value set conversion rules and then compare the result to the double.

        Show
        Phil Steitz added a comment - It is not obvious to me that the third example should succeed. An argument could be made that the existing code does the right thing when passed a double and a float, which is to convert the float according to the value set conversion rules and then compare the result to the double.
        Hide
        Sebb added a comment -

        Yes, I think the 3rd example is not relevant to the immediate problem (although indirectly it was what alerted me to the problem).

        Though it may perhaps be worthwhile noting the conversion issue somewhere in the Javadoc - perhaps at class level?

        Show
        Sebb added a comment - Yes, I think the 3rd example is not relevant to the immediate problem (although indirectly it was what alerted me to the problem). Though it may perhaps be worthwhile noting the conversion issue somewhere in the Javadoc - perhaps at class level?
        Hide
        Sebb added a comment -

        Just had a thought:

        I don't think the exactly same code can be added to 2.x, because the semantics of some of the double methods has changed.
        It would be confusing if floats behaved differently from doubles!

        So I propose to rework the fix for 2.x to preserve the deprecated behaviour and deprecated tags.

        OK?

        Show
        Sebb added a comment - Just had a thought: I don't think the exactly same code can be added to 2.x, because the semantics of some of the double methods has changed. It would be confusing if floats behaved differently from doubles! So I propose to rework the fix for 2.x to preserve the deprecated behaviour and deprecated tags. OK?
        Hide
        Phil Steitz added a comment -

        Yes, you are right. Needs to be reworked slightly for 2.x.

        Show
        Phil Steitz added a comment - Yes, you are right. Needs to be reworked slightly for 2.x.
        Hide
        Gilles added a comment -

        MathUtils.equals(double, double) does not work properly for floats.

        There is no equals(float,float) so float parameters are automatically promoted to double. However, that is not necessarily appropriate, given that the ULP for a double is much smaller than the ULP for a float.

        This is not a bug, but expected behaviour. But I certainly agree that it does not hurt to mention the conversion issue for the unwary.

        However, shouldn't there be emphasis, in the user guide, that CM is a "double" precision library? A quick poll of the code gives the following numbers:
        Occurrences of the string "float ": 41 (roughly half of which were introduced with this patch)
        Occurrences of the string "double ": 4061

        Also, I'm curious as to what use case you were having that requires comparing float numbers.

        Finally if we want to help users avoid such pitfalls, I think that we should consider refactoring MathUtils so that similar methods that should behave differently for different types are in different classes (exactly as with classes Float and Double). Thus, we should create MathUtilsDouble (or assume that the current MathUtils is for double utilities) and MathUtilsFloat.

        Show
        Gilles added a comment - MathUtils.equals(double, double) does not work properly for floats. There is no equals(float,float) so float parameters are automatically promoted to double. However, that is not necessarily appropriate, given that the ULP for a double is much smaller than the ULP for a float. This is not a bug, but expected behaviour. But I certainly agree that it does not hurt to mention the conversion issue for the unwary. However, shouldn't there be emphasis, in the user guide, that CM is a "double" precision library? A quick poll of the code gives the following numbers: Occurrences of the string "float ": 41 (roughly half of which were introduced with this patch) Occurrences of the string "double ": 4061 Also, I'm curious as to what use case you were having that requires comparing float numbers. Finally if we want to help users avoid such pitfalls, I think that we should consider refactoring MathUtils so that similar methods that should behave differently for different types are in different classes (exactly as with classes Float and Double ). Thus, we should create MathUtilsDouble (or assume that the current MathUtils is for double utilities) and MathUtilsFloat .
        Hide
        Sebb added a comment -

        Many of the double-only methods work OK with widened float parameters.

        Where this is not the case, we either need to fix the discrepancy (as per this JIRA) or document the restriction.

        I don't see the point of separating the utilities into two different classes, but if the consensus is that we should, then we need to make sure that the same methods are present in both, even if the double method works perfectly well, otherwise it would be even more confusing for users (where do I find the method?).

        But I think the main issue is that having a separate class would force users to edit and recompile to take advantage of any fixes such as this one.

        Show
        Sebb added a comment - Many of the double-only methods work OK with widened float parameters. Where this is not the case, we either need to fix the discrepancy (as per this JIRA) or document the restriction. I don't see the point of separating the utilities into two different classes, but if the consensus is that we should, then we need to make sure that the same methods are present in both, even if the double method works perfectly well, otherwise it would be even more confusing for users (where do I find the method?). But I think the main issue is that having a separate class would force users to edit and recompile to take advantage of any fixes such as this one.
        Hide
        Phil Steitz added a comment -

        I agree with Sebb on this one. I think it is a bug (the second example). The promotion separates the values that are indistinguishable as floats. Consistently with comments on MATH-474, equals(,) should identify indiscernibles. I also agree that it is better to just add the methods to MathUtils.

        Show
        Phil Steitz added a comment - I agree with Sebb on this one. I think it is a bug (the second example). The promotion separates the values that are indistinguishable as floats. Consistently with comments on MATH-474 , equals( , ) should identify indiscernibles. I also agree that it is better to just add the methods to MathUtils.
        Hide
        Sebb added a comment -

        Patches now applied.

        Show
        Sebb added a comment - Patches now applied.
        Hide
        Luc Maisonobe added a comment -

        Closing issue as it was included in version 2.2, which has been released

        Show
        Luc Maisonobe added a comment - Closing issue as it was included in version 2.2, which has been released

          People

          • Assignee:
            Unassigned
            Reporter:
            Sebb
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development