Add Ring[BigDecimal], modeled after Ring[BigInt] #553
Conversation
| @@ -13,8 +13,7 @@ import CMSFunctions.generateHashes | |||
| * We benchmark different `K` types as well as different input data streams. | |||
| */ | |||
| object CMSBenchmark { | |||
|
|
|||
| import CMSHasherImplicits.CMSHasherBigInt | |||
| import CMSHasherImplicits.{ CMSHasherBigInt, CMSHasherBigDecimal } | |||
There was a problem hiding this comment.
we should not need this. CMSHasherImplicits is legacy. Let's not add to it. Just add to the object CMSHasher.
| @@ -12,7 +12,7 @@ import scala.util.Random.nextString | |||
| */ | |||
|
|
|||
| object TopCMSBenchmark { | |||
| import CMSHasherImplicits.CMSHasherBigInt | |||
| import CMSHasherImplicits.{ CMSHasherBigInt, CMSHasherBigDecimal } | |||
There was a problem hiding this comment.
let's not add to CMSHasherImplicits
| @@ -1332,5 +1332,10 @@ object CMSHasherImplicits { | |||
| CMSHasher.hashBytes(a, b, width)(x.getBytes("UTF-8")) | |||
| } | |||
|
|
|||
| implicit object CMSHasherBigDecimal extends CMSHasher[BigDecimal] { | |||
There was a problem hiding this comment.
this should only be added to object CMSHasher see the comment on this object.
There was a problem hiding this comment.
I went a little too far in the "mechanised patch" thing. Should've had paid more attention. this and the two previous fixed.
Shouldn't we gut CMSHasherImplicits, move its contents to CMSHasher's companion, leave "implicit vals" in place for compatibility, and @deprecate the whole thing?
| @@ -139,6 +140,7 @@ object Ring extends GeneratedRingImplicits with ProductRings { | |||
| implicit val jshortRing: Ring[JShort] = JShortRing | |||
| implicit val longRing: Ring[Long] = LongRing | |||
| implicit val bigIntRing: Ring[BigInt] = BigIntRing | |||
| implicit val bigDecimalRing: Ring[BigDecimal] = BigDecimalRing | |||
There was a problem hiding this comment.
why didn't line 134 above work? Or, maybe it did here, but not for Semirgoup, Monoid, Group? If so, I guess we should fix that.
There was a problem hiding this comment.
Well. I didn't go that far. My initial motivation was that eventually I no longer need to provide my own Semigroup[BigDecimal]; as Ring[BigInt] was here and is used by `Semigroup[BigInt],Monoid[BigInt]``,``Group[BigInt]``, I just xerox'd the pattern.
| @@ -1332,5 +1332,10 @@ object CMSHasherImplicits { | |||
| CMSHasher.hashBytes(a, b, width)(x.getBytes("UTF-8")) | |||
| } | |||
|
|
|||
| implicit object CMSHasherBigDecimal extends CMSHasher[BigDecimal] { | |||
| override def hash(a: Int, b: Int, width: Int)(x: BigDecimal): Int = | |||
| CMSHasherString.hash(a, b, width)(x.underlying.toPlainString) | |||
There was a problem hiding this comment.
I'm afraid this is slow. Can you compare this to using x.underlying.scale and x.underling.unscaledValue? You could use the byte array from unscaledValue and perhaps multiply the scale by a before passing to the Array[Byte] hashing.
If this is not faster, we don't have to do it, but allocating strings is generally slow.
There was a problem hiding this comment.
D'oh. Yes of course. Done.
FWIW, x.underlying.unscaledValue.toByteArray does allocate a temporary array. From what I understand HotSpot'JIT is doing these days, inline + lifetime escape analysis should eventually make that a (very cheap) stack alloc if that bit of code is hot.
(didn't check the C2 output to confirm)
cchepelov
force-pushed
the
pr-bigdecimal
branch
2 times, most recently
from
1eaff3a to
22a7004
Compare
|
looks like the tests fail due to absurdly huge numbers being generated by /cc @non Will need a custom |
| testBatchedMonoid[BigDecimal]("BigDecimal", 1)(arbReasonableBigDecimals, implicitly[Monoid[BigDecimal]]) | ||
| testBatchedMonoid[BigDecimal]("BigDecimal", 10)(arbReasonableBigDecimals, implicitly[Monoid[BigDecimal]]) | ||
| testBatchedMonoid[BigDecimal]("BigDecimal", 100)(arbReasonableBigDecimals, implicitly[Monoid[BigDecimal]]) | ||
| testBatchedMonoid[BigDecimal]("BigDecimal", 1000000)(arbReasonableBigDecimals, implicitly[Monoid[BigDecimal]]) |
There was a problem hiding this comment.
I'm not really happy with the syntax here (the "implicit" on line 30, and maybe the whole of line 30, is spurious). Not quite sure how to do it better. I tried to hide the low-priority implicits brought in by the Arbitrary object, but didn't quite succeed.
Current coverage is 63.90% (diff: 30.76%)@@ develop #553 diff @@
==========================================
Files 110 110
Lines 4423 4433 +10
Methods 4022 4043 +21
Messages 0 0
Branches 360 351 -9
==========================================
+ Hits 2824 2833 +9
- Misses 1599 1600 +1
Partials 0 0
|
| testBatchedMonoid[BigDecimal]("BigDecimal", 10) | ||
| testBatchedMonoid[BigDecimal]("BigDecimal", 100) | ||
| testBatchedMonoid[BigDecimal]("BigDecimal", 1000000) | ||
| testBatchedMonoid[BigDecimal]("BigDecimal", 1)(arbReasonableBigDecimals, implicitly[Monoid[BigDecimal]]) |
There was a problem hiding this comment.
I don't think you should need this. I think the issue is that you have the Arbitrary both implicit in BaseProperties imported as well as arbReasonableBigDecimals.
I'd rather not make the BaseProperties one implicit. That should fix the issue. That makes it opt in, as you did here, but I think that is more conservative since we don't "own" either Arbitrary or BigDecimal.
(Added a custom generator for BigDecimals, to reduce somewhat the risk of underflow)
cchepelov
force-pushed
the
pr-bigdecimal
branch
from
c0a2d98 to
d83922c
Compare
|
D'oh! of course.
Interesting that scalac discards the confusing implicit to end up using
the low-priority one instead, with no warning.
Just pushed a cleaned-up version, with squashed commits. Thanks for your
review!
|
|
👍 merge when green. |
Algebird provides rings (and semigroups, etc.) for a set of numeric types, including BigInt. It doesn't (so far) for BigDecimal.
This PR adds Ring[BigDecimal](modeled rather mechanically after Ring[BigInt]) and friends.