Fix precision issues with Money #268
Conversation
| USD(5) - USD(2) should be(USD(3)) | ||
| USD(5).minus(USD(2)) should be(USD(3)) | ||
| } | ||
|
|
||
| it should "return proper result when dividing like currencies with no MoneyContext in scope" in { | ||
| it should "return a proper result with no additional precision loss when multiplying by a primitive numeric" in { |
There was a problem hiding this comment.
I wonder if we should make this a property based test?
There was a problem hiding this comment.
That's a good suggestion in general for test, scala check could be appropriate for that
There was a problem hiding this comment.
I'll add a Scalacheck test.
There was a problem hiding this comment.
So, the Scalachecks revealed that this issue can also exist with BigDecimal.
val x = BigDecimal(3)
val m = BigDecimal(2)
m / x + m / x = 1.3333333333333333333333333333333334
(m / x) * 2 = 1.333333333333333333333333333333333Interestingly, that same calculation with Double works correctly
val x = 3d
val m = 2d
m / x + m / x = 1.3333333333333333
(m / x) * 2 = 1.3333333333333333I can resolve this by using an approximation with a tolerance of USD(1e-32) to the comparison. Most use cases will require much less than 32 digits and will typically round to the nearest penny (or formattedDecimal for the Currency), so this is still a big improvement over the 16/17 digits we get with Double math.
The multiplication use case noted in the original issue does not have this problem, so no approximation was needed there.
I'll have an updated PR in a moment.
There was a problem hiding this comment.
That's great, we should probably extend scalacheck's usage to other tests
Also temporarily reverted to Scalariform 1.5.1
garyKeorkunian
force-pushed
the
wip-money-precision
branch
from
1fb4e97
to
e85dc25
Compare
|
LGTM. Are we good to merge this? |
|
Any thoughts on that Checks issue I mentioned? |
Also temporarily reverted to Scalariform 1.5.1
Fixes #265