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.

Sign up for GitHub

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

Jump to bottom

Fix precision issues with Money #268

Merged
merged 1 commit into from
Jun 21, 2017
Merged

Fix precision issues with Money #268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion project/plugins.sbt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ addSbtPlugin("org.scala-native" % "sbt-scalajs-crossproject" % "0.1.0")

addSbtPlugin("org.scala-native" % "sbt-scala-native" % "0.2.1")

addSbtPlugin("org.scalariform" % "sbt-scalariform" % "1.6.0")
addSbtPlugin("org.scalariform" % "sbt-scalariform" % "1.5.1")

addSbtPlugin("com.typesafe.sbt" % "sbt-osgi" % "0.8.0")

Expand Down
10 changes: 10 additions & 0 deletions shared/src/main/scala/squants/market/CurrencyExchangeRate.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,16 @@ case class CurrencyExchangeRate(base: Money, counter: Money) extends Ratio[Money
/** convert */
def *(money: Money) = convert(money)


/**
* Override methods from Ratio to ensure BigDecimal precision math is applied
*
* @param m Money
* @return
*/
override def convertToBase(m: Money): Money = base * (m / counter)
override def convertToCounter(m: Money): Money = counter * (m / base)

/**
* Returns the rate formatted in as standard FX Quote"
* @return
Expand Down
33 changes: 30 additions & 3 deletions shared/src/main/scala/squants/market/Money.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,15 @@ final class Money private (val amount: BigDecimal)(val currency: Currency)
*
* @return String
*/
override def toString: String = amount.toString + " " + currency.code
override def toString: String = amount.underlying.stripTrailingZeros.toString + " " + currency.code

/**
* Converts the amount to the given currency and returns a string formatted with the original precision and the currency code
*
* @param c Currency
* @param context MoneyContext required for conversion
* @return
*/
def toString(c: Currency)(implicit context: MoneyContext): String = in(c).toString

/**
Expand Down Expand Up @@ -127,7 +134,17 @@ final class Money private (val amount: BigDecimal)(val currency: Currency)
* @param that BigDecimal
* @return Money
*/
def *(that: BigDecimal): Money = new Money(amount * that)(currency)
def times(that: BigDecimal): Money = new Money(amount * that)(currency)
def *(that: BigDecimal): Money = times(that)

/**
* Overrides Quantity.times to ensure BigDecimal math is performed
*
* @param that Double
* @return Quantity
*/
override def times(that: Double): Money = new Money(amount * that)(currency)
override def *(that: Double): Money = times(that)

/**
* Multiplies this money by that [[squants.market.CurrencyExchangeRate]] and returns the equal value in the other currency.
Expand All @@ -145,7 +162,17 @@ final class Money private (val amount: BigDecimal)(val currency: Currency)
* @param that BigDecimal
* @return Money
*/
def /(that: BigDecimal): Money = new Money(amount / that)(currency)
def divide(that: BigDecimal): Money = new Money(amount / that)(currency)
def /(that: BigDecimal): Money = divide(that)

/**
* Overrides Quantity.divide to ensure BigDecimal math is performed
*
* @param that Double
* @return Quantity
*/
override def divide(that: Double): Money = new Money(amount / that)(currency)
override def /(that: Double): Money = divide(that)

/**
* Integer divides this money by that BigDecimal and returns the remainder
Expand Down
3 changes: 1 addition & 2 deletions shared/src/test/scala/squants/QuantityChecks.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@ import org.scalacheck.Gen
trait QuantityChecks {

type TestData = Int
val posNum = Gen.posNum[TestData]
val posNum: Gen[TestData] = Gen.posNum[TestData]
val tol = 1e-13
implicit val tolTime = Seconds(tol)

}
34 changes: 28 additions & 6 deletions shared/src/test/scala/squants/market/MarketChecks.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,45 @@ import org.scalacheck.Prop._
object MarketChecks extends Properties("Market") with QuantityChecks {

property("Money + Money = Money") = forAll(posNum, posNum) { (a: TestData, b: TestData) ⇒
USD(a + b) == USD(a.toDouble) + USD(b.toDouble)
USD(a + b) == USD(a) + USD(b)
}

property("Money - Money = Money") = forAll(posNum, posNum) { (a: TestData, b: TestData) ⇒
USD(a - b) == USD(a.toDouble) - USD(b.toDouble)
USD(a - b) == USD(a) - USD(b)
}

property("Money * Double = Money") = forAll(posNum, posNum) { (a: TestData, b: TestData) ⇒
USD(a * b) == USD(a.toDouble) * b.toDouble
USD(a * b) == USD(a) * b
}

property("Money / Double = Money") = forAll(posNum, posNum) { (a: TestData, b: TestData) ⇒
USD(a.toDouble / b) == USD(a.toDouble) / b.toDouble
USD(BigDecimal(a) / b) == USD(a) / b
}

property("Money = ExchangeRate * Money") = forAll(posNum, posNum) { (a: TestData, b: TestData) ⇒
USD(b.toDouble / a.toDouble) == CurrencyExchangeRate(USD(1), JPY(a.toDouble)) * JPY(b.toDouble) &&
USD(b.toDouble / a.toDouble) == JPY(b.toDouble) * CurrencyExchangeRate(USD(1), JPY(a.toDouble))
USD(BigDecimal(b) / a) == CurrencyExchangeRate(USD(1), JPY(a)) * JPY(b) &&
USD(BigDecimal(b) / a) == JPY(b) * CurrencyExchangeRate(USD(1), JPY(a))
}

property("(variations of) Money * Double + Money * Double = (Money * Double) * 2") = forAll(posNum, posNum) { (a: TestData, b: TestData) ⇒
val m = USD(a.toDouble)
val x = b.toDouble

x * m + x * m == ((m * x) * 2) &&
m * x + x * m == ((m * x) * 2) &&
m * x + m * x == ((m * x) * 2) &&
x * m + m * x == ((m * x) * 2) &&
x * m + x * m == ((x * m) * 2) &&
m * x + x * m == ((x * m) * 2) &&
m * x + m * x == ((x * m) * 2) &&
x * m + m * x == ((x * m) * 2)
}

property("Money / Double + Money / Double = (Money / Double) * 2") = forAll(posNum, posNum) { (a: TestData, b: TestData) ⇒
implicit val tolUSD = USD(1e-32)
val m = if (a > 0) USD(a.toDouble) else USD(1)
val x = if (b > 0) b.toDouble else 1d

(m / x + m / x) ~= ((m / x) * 2)
}
}
30 changes: 26 additions & 4 deletions shared/src/test/scala/squants/market/MoneySpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ class MoneySpec extends FlatSpec with Matchers {
x != y should be(right = true)
}

it should "return proper result on max/min operation with an implicit MoneyContext in scope" in {
it should "return a proper result on max/min operation with an implicit MoneyContext in scope" in {
val r1 = CurrencyExchangeRate(USD(1), JPY(100))
val r2 = CurrencyExchangeRate(USD(1), EUR(.75))
implicit val moneyContext = MoneyContext(USD, defaultCurrencySet, List(r1, r2))
Expand All @@ -152,17 +152,39 @@ class MoneySpec extends FlatSpec with Matchers {
y.moneyMin(x) should be(y)
}

it should "return proper result when adding like currencies with no MoneyContext in scope" in {
it should "return a proper result when adding like currencies with no MoneyContext in scope" in {
USD(1) + USD(2) should be(USD(3))
USD(1).plus(USD(2)) should be(USD(3))
}

it should "return proper result when subtracting like currencies with no MoneyContext in scope" in {
it should "return a proper result when subtracting like currencies with no MoneyContext in scope" in {
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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should make this a property based test?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good suggestion in general for test, scala check could be appropriate for that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a Scalacheck test.

Copy link
Contributor Author

@garyKeorkunian garyKeorkunian Jun 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Interestingly, that same calculation with Double works correctly

val x = 3d
val m = 2d

m / x + m / x = 1.3333333333333333
(m / x) * 2 = 1.3333333333333333

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's great, we should probably extend scalacheck's usage to other tests

val x: Double = 4992
val m: Money = EUR(9709.6)

x * m + x * m should be((m * x) * 2)
m * x + x * m should be((m * x) * 2)
m * x + m * x should be((m * x) * 2)
x * m + m * x should be((m * x) * 2)

x * m + x * m should be((x * m) * 2)
m * x + x * m should be((x * m) * 2)
m * x + m * x should be((x * m) * 2)
x * m + m * x should be((x * m) * 2)
}

it should "return a proper result with no additional precision loss when dividing by a primitive number" in {
val d: Double = 4992d
val m: Money = EUR(9709.6)

m / d + m / d should be((m / d) * 2)
}

it should "return a proper result when dividing like currencies with no MoneyContext in scope" in {
USD(10) / USD(2) should be(5)
USD(10).divide(USD(2)) should be(5)
}
Expand Down