Add choose methods for both Java and Scala BigDecimal. #670
Conversation
In addition to implicit Choose instances for scala.math.BigDecimal and
java.math.BigDecimal we also include explicit constructor methods, since users
may wish to be explicit about the scale they want. We may want to put those
methods directly on Gen, currently the ergonomics of using this are a bit bad:
Gen.Choose.chooseBigDecimalScale(100).choose(0, 1)
The BigDecimal generation is not yet tested. That will also be added in a
follow up. This also optimizes the BigInt generator a bit and generalizes it to
java.math.BigInteger to support that as well.
Addresses typelevel#631, typelevel#637, and typelevel#664
| case n if n > 0 => throw new IllegalBoundsError(low, high) | ||
| case 0 => Gen.const(low) | ||
| case _ => /* n < 0 */ | ||
| val s = (low.scale max high.scale) max minScale |
There was a problem hiding this comment.
It's less explicit, but couldn't the user just specify the minimum desired scale on either of the parameters for high and low to the Gen.choose method? For example,
forAll(Gen.choose(BigDecimal(-2).pow(29).setScale(10),
BigDecimal( 2).pow(29))) { (d: BigDecimal) =>
d.scale >= 10
}There was a problem hiding this comment.
This also came up in the discussion in #637, but I didn't conclude that there's a justification for the minScale parameter to the Choose factory.
There was a problem hiding this comment.
You could accomplish this by just changing this line in your pr:
val s = (low.scale max high.scale) max (low.scale min high.scale)There was a problem hiding this comment.
I don't understand what that last line accomplishes, the LHS (low.scale max high.scale) will always be chosen over the RHS (low.scale min high.scale).
My experience here is that many people using BigDecimal don't necessarily think about scale. For example, code like Gen.choose(BigDecimal(0), BigDecimal(1)) will only produce two possible values, but this issue won't necessarily be detected by authors, leading to unexpectedly weak tests.
Using the implicit scale in BigDecimal has tripped people up, especially given how tricky it is to reason about. For example, authors might think that Gen.choose(BigDecimal(0.0000), BigDecimal(1.0000)) will give them a scale of 4, but actually it will be 1 due to a different treatment of String and Double constructors:
scala> val x = BigDecimal(0.0000)
val x: scala.math.BigDecimal = 0.0
scala> x.scale
val res0: Int = 1
scala> val y = BigDecimal("0.0000")
val y: scala.math.BigDecimal = 0.0000
scala> y.scale
val res1: Int = 4I'm open to simplifying the API a bit but I strongly feel like we should have a relatively rigorous minScale, even if it's hardcoded. It's possible to truncate a generated BigDecimal for less precision but the reverse is not possible, so I feel like more precision (to a point) is a better default.
|
Should we go ahead with this for 1.15.0? |
|
I will try to take a look at this again. I had written some code that experimented with this. Hopefully, it can help me remember where I left off. |
|
@ashawley Ping? If I made the methods taking |
|
I'm going to go ahead and make the change I suggested -- it seems like a reasonable step to do regardless. |
|
Thanks, Erik! |
In addition to implicit Choose instances for scala.math.BigDecimal and
java.math.BigDecimal we also include explicit constructor methods, since users
may wish to be explicit about the scale they want. We may want to put those
methods directly on Gen, currently the ergonomics of using this are a bit bad:
The BigDecimal generation is not yet tested. That will also be added in a
follow up. This also optimizes the BigInt generator a bit and generalizes it to
java.math.BigInteger to support that as well.
Addresses #631, #637, and #664