Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BigDecimal.isValidDouble behaves unexpectedly #6699

Closed
scabug opened this issue Nov 21, 2012 · 13 comments
Closed

BigDecimal.isValidDouble behaves unexpectedly #6699

scabug opened this issue Nov 21, 2012 · 13 comments
Assignees
Milestone

Comments

@scabug
Copy link

scabug commented Nov 21, 2012

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

@scabug
Copy link
Author

scabug commented Nov 21, 2012

Imported From: https://issues.scala-lang.org/browse/SI-6699?orig=1
Reporter: Samuel Halliday (fommil)
Affected Versions: 2.10.0-RC2

@scabug
Copy link
Author

scabug commented Dec 6, 2012

Sergii Zashchelkin (sergz72) said (edited on Dec 6, 2012 11:09:27 AM UTC):
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
}

@scabug
Copy link
Author

scabug commented Dec 6, 2012

@acruise said:
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. :)

@scabug
Copy link
Author

scabug commented Dec 8, 2012

Sergii Zashchelkin (sergz72) said:
Hi Alex

I've submitted a pull request for #6699 fix.

Thanks,
Sergii

@scabug
Copy link
Author

scabug commented Jan 23, 2013

@adriaanm said:
scala/scala#1725 was closed due to test failures

@scabug
Copy link
Author

scabug commented Oct 14, 2013

@Ichoran said:
The problem here is exacerbated because someone believed javadoc that said you should use BigDecimal.valueOf to create BigDecimals. (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.

@scabug
Copy link
Author

scabug commented Jan 3, 2014

@adriaanm said:
Should we deprecate the dubious part of the api?

@scabug
Copy link
Author

scabug commented Jan 15, 2014

@adriaanm said:
scala/scala#3316

@scabug scabug closed this as completed Jan 15, 2014
@scabug scabug added this to the 2.11.0-M8 milestone Apr 7, 2017
@plokhotnyuk
Copy link

@adriaanm @Ichoran why it was closed without a fix?

scala> BigDecimal(10.1: Double).isValidDouble
<console>:12: warning: method isValidDouble in class BigDecimal is deprecated (since 2.11.0): Validity has distinct meanings. Use `isExactDouble`, `isBinaryDouble`, or `isDecimalDouble` instead.
       BigDecimal(10.1: Double).isValidDouble
                                ^
res0: Boolean = false

scala> BigDecimal(10.1: Double).isExactDouble
res1: Boolean = false

scala> BigDecimal(10.1: Double).isExactFloat
res2: Boolean = false

scala> BigDecimal(10.1: Double).isBinaryDouble
res3: Boolean = false

scala> BigDecimal(10.1: Double).isDecimalDouble
res4: Boolean = true

@adriaanm
Copy link
Contributor

Not sure what's going on the PR description mentioned this bug as fixed: scala/scala#3316

@adriaanm
Copy link
Contributor

I suppose it's actually down to the choice of the number, and explained by https://stackoverflow.com/questions/21895756/why-are-floating-point-numbers-inaccurate

scala> BigDecimal(10.5: Double).isExactDouble
res12: Boolean = true

@adriaanm
Copy link
Contributor

scala> 10.1+10.1+10.1
res13: Double = 30.299999999999997

scala> 10.5+10.5+10.5
res14: Double = 31.5

@Ichoran
Copy link

Ichoran commented Sep 18, 2018

Maybe this will make things clear:

@ BigDecimal.exact(10.1) 
res3: BigDecimal = 10.0999999999999996447286321199499070644378662109375

@ BigDecimal(10.1) 
res4: BigDecimal = 10.1

@ res3.isExactDouble 
res5: Boolean = true

@ res3.isDecimalDouble 
res6: Boolean = false

@ res4.isExactDouble 
res7: Boolean = false

@ res4.isDecimalDouble 
res8: Boolean = true

Innocent-looking things like 10.1 do not have an exact representation as a binary decimal, so you have to choose whether you wanted the exact value of the Double (which you called 10.1 but isn't actually 10.1), or the standard decimal representation of the Double (which, in this case, is 10.1).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants