Scala Programming Language
  1. Scala Programming Language
  2. SI-6699

BigDecimal.isValidDouble behaves unexpectedly

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: Scala 2.10.0-RC2
    • Fix Version/s: Scala 2.11.0-M8
    • Component/s: Misc Library
    • Labels:
      None
    • Environment:

      OS X Mountain Lion

      Description

      (Sorry, I can't create anything less than a BLOCKER bug report.)

      I would expect that creating a `BigDecimal` from a `Double` would produce a `BigDecimal` that returns `BigDecimal.isValidDouble = true`, e.g.

      (Note: in the following, we don't care about loss of precision when inputting a double literal, the fact that whatever is stored is represented as a `Double` is enough)

      println(BigDecimal(10.1: Double).isValidDouble) // false

      but the above actually prints `false`.

      WORKAROUND: I use my own replacement for `.isValidDouble` which is to create a new BigDecimal from the output of a `toDouble` conversion

      val sd = BigDecimal(10.1: Double)
      println(sd == BigDecimal(sd.toDouble)) // true

      If the `BigDecimal` -> `Double` conversion loses precision, then this test fails. e.g.

      val ssd = BigDecimal("1.0000000000000003")
      println(ssd == BigDecimal(ssd.toDouble)) // false

      as expected.

        Activity

        Hide
        Sergii Zashchelkin added a comment - - edited

        Hi

        I have no rights to commit to Scala git repository, so please find below the working code for BigDecimal.isValidDouble:

        def isValidDouble =

        { val d = toDouble !d.isInfinity && bigDecimal.compareTo(new java.math.BigDecimal(jl.Double.toString(d), mc)) == 0 }
        Show
        Sergii Zashchelkin added a comment - - edited Hi I have no rights to commit to Scala git repository, so please find below the working code for BigDecimal.isValidDouble: def isValidDouble = { val d = toDouble !d.isInfinity && bigDecimal.compareTo(new java.math.BigDecimal(jl.Double.toString(d), mc)) == 0 }
        Hide
        Alex Cruise added a comment -

        Hi Sergii,

        The proper procedure in this case is to fork, create a branch, fix it in your branch, add some tests, make sure the main Scala tests pass as well as your new ones, then submit a pull request. I'd be happy to review your contribution once it's up on github.

        Show
        Alex Cruise added a comment - Hi Sergii, The proper procedure in this case is to fork, create a branch, fix it in your branch, add some tests, make sure the main Scala tests pass as well as your new ones, then submit a pull request. I'd be happy to review your contribution once it's up on github.
        Hide
        Sergii Zashchelkin added a comment -

        Hi Alex

        I've submitted a pull request for SI-6699 fix.

        Thanks,
        Sergii

        Show
        Sergii Zashchelkin added a comment - Hi Alex I've submitted a pull request for SI-6699 fix. Thanks, Sergii
        Hide
        Adriaan Moors added a comment -

        https://github.com/scala/scala/pull/1725 was closed due to test failures

        Show
        Adriaan Moors added a comment - https://github.com/scala/scala/pull/1725 was closed due to test failures
        Hide
        Rex Kerr added a comment -

        The problem here is exacerbated because someone believed javadoc that said you should use `BigDecimal.valueOf` to create `BigDecimal`s. (Unfortunately the justification given in javadocs is that it goes through the string representation!)

        This keeps you consistent with the printed-out version of your doubles, but it doesn't keep you consistent in any other way. All your internal numbers may be randomly different from each other (after a bit of math).

        I recommend switching to purely non-string-based conversions wherever possible.

        Here's an example of the problem as it currently exists:

        val l = (1L << 62L)
        val d = l.toDouble
        l == d // true
        BigDecimal(l) == BigDecimal(d)  // false?!
        l == BigDecimal(l)  // true
        d == BigDecimal(d)  // false?!
        

        To recover transitivity of equals, it is necessary to switch away from taking the `doubleValue.toString` representation as the canonical one for initializing `BigDecimal`.

        Unfortunately, if people are relying upon this strange string conversion thing for BigDecimal, changing it could break e.g. financial software. That's generally something you don't want broken.

        Show
        Rex Kerr added a comment - The problem here is exacerbated because someone believed javadoc that said you should use `BigDecimal.valueOf` to create `BigDecimal`s. (Unfortunately the justification given in javadocs is that it goes through the string representation!) This keeps you consistent with the printed-out version of your doubles, but it doesn't keep you consistent in any other way. All your internal numbers may be randomly different from each other (after a bit of math). I recommend switching to purely non-string-based conversions wherever possible. Here's an example of the problem as it currently exists: val l = (1L << 62L) val d = l.toDouble l == d // true BigDecimal(l) == BigDecimal(d) // false?! l == BigDecimal(l) // true d == BigDecimal(d) // false?! To recover transitivity of equals, it is necessary to switch away from taking the `doubleValue.toString` representation as the canonical one for initializing `BigDecimal`. Unfortunately, if people are relying upon this strange string conversion thing for BigDecimal, changing it could break e.g. financial software. That's generally something you don't want broken.
        Hide
        Adriaan Moors added a comment -

        Should we deprecate the dubious part of the api?

        Show
        Adriaan Moors added a comment - Should we deprecate the dubious part of the api?
        Show
        Adriaan Moors added a comment - https://github.com/scala/scala/pull/3316

          People

          • Assignee:
            Rex Kerr
            Reporter:
            Samuel Halliday
          • Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development