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

Add choose methods for both Java and Scala BigDecimal. #670

Merged
merged 4 commits into from
Oct 24, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
50 changes: 40 additions & 10 deletions jvm/src/test/scala/org/scalacheck/GenSpecification.scala
Original file line number Diff line number Diff line change
Expand Up @@ -124,25 +124,55 @@ object GenSpecification extends Properties("Gen") with GenSpecificationVersionSp
}
}

property("choose-big-int") = forAll(
nonEmptyContainerOf[Array, Byte](arbitrary[Byte]).map(BigInt(_)),
val manualBigInt: Gen[BigInt] =
nonEmptyContainerOf[Array, Byte](arbitrary[Byte]).map(BigInt(_))
) { (l: BigInt, h: BigInt) =>
Try(choose(l, h)) match {
case Success(g) => forAll(g) { x => l <= x && x <= h }
case Failure(e: Choose.IllegalBoundsError[_]) => Prop(l > h)
case Failure(e) => throw e

property("choose-big-int") =
forAll(manualBigInt, manualBigInt) { (l: BigInt, h: BigInt) =>
Try(choose(l, h)) match {
case Success(g) => forAll(g) { x => l <= x && x <= h }
case Failure(e: Choose.IllegalBoundsError[_]) => Prop(l > h)
case Failure(e) => throw e
}
}

property("choose-java-big-int") =
forAll(manualBigInt, manualBigInt) { (x0: BigInt, y0: BigInt) =>
val (x, y) = (x0.bigInteger, y0.bigInteger)
Try(choose(x, y)) match {
case Success(g) => forAll(g) { n => x.compareTo(n) <= 0 && y.compareTo(n) >= 0 }
case Failure(e: Choose.IllegalBoundsError[_]) => Prop(x.compareTo(y) > 0)
case Failure(e) => throw e
}
}
}

property("Gen.choose(BigInt( 2^(2^18 - 1)), BigInt(-2^(2^18 - 1)))") = {
val (l, h) = (BigInt(-2).pow(262143),
BigInt( 2).pow(262143))
val (l, h) = (BigInt(-2).pow(262143), BigInt(2).pow(262143))
Prop.forAllNoShrink(Gen.choose(l, h)) { x =>
l <= x && x <= h
}
}

property("choose-big-decimal") =
forAll { (x0: Double, y0: Double) =>
val (x, y) = (BigDecimal(x0), BigDecimal(y0))
Try(choose(x, y)) match {
case Success(g) => forAll(g) { n => x <= n && n <= y }
case Failure(e: Choose.IllegalBoundsError[_]) => Prop(x > y)
case Failure(e) => throw e
}
}

property("choose-java-big-decimal") =
forAll { (x0: Double, y0: Double) =>
val (x, y) = (BigDecimal(x0).bigDecimal, BigDecimal(y0).bigDecimal)
Try(choose(x, y)) match {
case Success(g) => forAll(g) { n => x.compareTo(n) <= 0 && y.compareTo(n) >= 0 }
case Failure(e: Choose.IllegalBoundsError[_]) => Prop(x.compareTo(y) > 0)
case Failure(e) => throw e
}
}

property("choose-xmap") = {
implicit val chooseDate: Choose[Date] =
Choose.xmap[Long, Date](new Date(_), _.getTime)
Expand Down
150 changes: 136 additions & 14 deletions src/main/scala/org/scalacheck/Gen.scala
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import scala.collection.mutable.ArrayBuffer
import scala.concurrent.duration.{Duration, FiniteDuration}

import java.util.{ Calendar, UUID }
import java.nio.ByteBuffer
import java.math.{BigInteger, BigDecimal => JavaDecimal}

sealed abstract class Gen[+T] extends Serializable { self =>

Expand Down Expand Up @@ -410,6 +410,65 @@ object Gen extends GenArities with GenVersionSpecific {
}
}

/**
* Generate a random BigInt within [lower, lower + span).
*
* Note that unlike the choose method, whose bounds are inclusive,
* this method's upper bound is exclusive. We determine how many
* random bits we need (bitLen), and then round up to the nearest
* number of bytes (byteLen). We generate the bytes, possibly
* truncating the most significant byte (bytes(0)) if bitLen is
* not evenly-divisible by 8.
*
* Finally, we check to see if the BigInt we ended up with is in
* our range. If it is not, we restart this method. The likelihood
* of needing to restart depends on span. In the worst case we
* have almost a 50% chance of this (which occurs when span is a
* power of 2 + 1) and in the best case we never restart (which
* occurs when span is a power of 2).
*/
private def chBigInteger(lower: BigInteger, span: BigInteger, seed0: Seed): R[BigInteger] = {
val bitLen = span.bitLength
val byteLen = (bitLen + 7) / 8
val bytes = new Array[Byte](byteLen)
var seed = seed0
var i = 0
while (i < bytes.length) {

// generate a random long value (i.e. 8 random bytes)
val (x0, seed1) = seed.long
var x = x0
seed = seed1

// extract each byte in turn and add them to our byte array
var j = 0
while (j < 8 && i < bytes.length) {
val b = (x & 0xff).toByte
bytes(i) = b
x = x >>> 8
i += 1
j += 1
}
}

// we may not need all 8 bits of our most significant byte. if
// not, mask off any unneeded upper bits.
val bitRem = bitLen & 7
if (bitRem != 0) {
val mask = 0xff >>> (8 - bitRem)
bytes(0) = (bytes(0) & mask).toByte
}

// construct a BigInteger and see if its valid. if so, we're
// done. otherwise, we need to restart using our new seed.
val big = new BigInteger(1, bytes)
if (big.compareTo(span) < 0) {
r(Some(big.add(lower)), seed)
} else {
chBigInteger(lower, span, seed)
}
}

implicit val chooseLong: Choose[Long] =
new Choose[Long] {
def choose(low: Long, high: Long): Gen[Long] =
Expand Down Expand Up @@ -449,22 +508,85 @@ object Gen extends GenArities with GenVersionSpecific {

implicit object chooseBigInt extends Choose[BigInt] {
def choose(low: BigInt, high: BigInt): Gen[BigInt] =
if (low > high) throw new IllegalBoundsError(low, high)
else if (low == high) low
else {
val range = high - low
Gen.containerOfN[Array, Long](
(range.bitLength + 64 - 1) / 64,
Gen.choose(Long.MinValue, Long.MaxValue)
).map(longs => {
longs(0) = longs(0) & (-1L >>> (64 - range.bitLength % 64))
val bb = ByteBuffer.allocate(longs.length * 8)
longs.foreach(bb.putLong)
BigInt(1, bb.array()) + low
}).filter(n => high >= n)
chooseBigInteger
.choose(low.bigInteger, high.bigInteger)
.map(BigInt(_))
}

implicit object chooseBigInteger extends Choose[BigInteger] {
def choose(low: BigInteger, high: BigInteger): Gen[BigInteger] =
(low compareTo high) match {
case n if n > 0 => throw new IllegalBoundsError(low, high)
case 0 => Gen.const(low)
case _ => /* n < 0 */
val span = high.subtract(low).add(BigInteger.ONE)
gen((_, seed) => chBigInteger(low, span, seed))
}
}

/**
* Choose a BigDecimal number between two given numbers.
*
* The minimum scale used will be 34. That means that the
* fractional part will have at least 34 digits (more if one of
* the given numbers has a scale larger than 34).
*
* The minimum scale was chosen based on Scala's default scale for
* expanding infinite fractions:
*
* BigDecimal(1) / 3 // 0.3333333333333333333333333333333333
*
* See chooseBigDecimalScale for more information about scale.
*/
implicit val chooseBigDecimal: Choose[BigDecimal] =
chooseBigDecimalScale(minScale = 34)

/**
* The "scale" of a decimal number refers to the number of digits
* in the fractional part. For example, 3.0000 has a scale of 4.
*
* We can generate an arbitrary number of digits in the decimal
* expansion of a number, so if a user calls choose(0, 1) we need
* to decide "how much" work to do. The minScale ensures that we
* do "enough" work to generate interesting numbers.
*
* The implicit instance fixes this value, but since users may
* want to use other scales we expose this method as well.
*/
private[this] def chooseBigDecimalScale(minScale: Int): Choose[BigDecimal] =
new Choose[BigDecimal] {
private val c = chooseJavaBigDecimalScale(minScale)
def choose(low: BigDecimal, high: BigDecimal): Gen[BigDecimal] =
c.choose(low.bigDecimal, high.bigDecimal).map(BigDecimal(_))
}

/**
* Choose a java.math.BigDecimal number between two given numbers.
*
* See chooseBigDecimal and chooseBigDecimalScale for more comments.
*/
implicit val chooseJavaBigDecimal: Choose[JavaDecimal] =
chooseJavaBigDecimalScale(minScale = 34)

/**
* See chooseBigDecimalScale for comments.
*/
private[this] def chooseJavaBigDecimalScale(minScale: Int): Choose[JavaDecimal] =
new Choose[JavaDecimal] {
def choose(low: JavaDecimal, high: JavaDecimal): Gen[JavaDecimal] =
(low compareTo high) match {
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@non non Jul 17, 2020

Choose a reason for hiding this comment

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

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 = 4

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

val x = if (low.scale < s) low.setScale(s) else low
val y = if (high.scale < s) high.setScale(s) else high
chooseBigInteger
.choose(x.unscaledValue, y.unscaledValue)
.map(n => new JavaDecimal(n, s))
}
}

/** Transform a Choose[T] to a Choose[U] where T and U are two isomorphic
* types whose relationship is described by the provided transformation
* functions. (exponential functor map) */
Expand Down