BigDecimal multiplication with Rational produces garbage digits within the requested precision. #4200
Comments
|
Yow, that's weird. I guess we're coercing the Rational to a low-precision float or something. Very strange. |
|
I'm re-porting some bits and pieces of the BigDecimal logic...and I'm getting close to having the precision you would expect, but the result is a little weird compared to MRI: I'll keep poking at it a bit this morning and at least post a patch. |
|
Well, here's what I came up with. This re-ports some coercion logic and precision-adjusting from MRI. It produces the above result for your example...but hangs (spinning forever) for a trivial high-exponent example from ruby/spec: At this point I'm not sure if I ported wrong (it looks ok) or if Java's kinda-crappy BigDecimal implementation is just woefully inefficient for this case. Here's the patch: diff --git a/core/src/main/java/org/jruby/ext/bigdecimal/RubyBigDecimal.java b/core/src/main/java/org/jruby/ext/bigdecimal/RubyBigDecimal.java
index 984a30c..e995654 100644
--- a/core/src/main/java/org/jruby/ext/bigdecimal/RubyBigDecimal.java
+++ b/core/src/main/java/org/jruby/ext/bigdecimal/RubyBigDecimal.java
@@ -424,10 +424,7 @@ public class RubyBigDecimal extends RubyNumeric {
return getVpValueWithPrec19(context, v, precision, must);
}
- private static RubyBigDecimal getVpRubyObjectWithPrec19Inner(ThreadContext context, RubyRational value) {
- return getVpRubyObjectWithPrec19Inner(context, value, getRoundingMode(context.runtime));
- }
-
+ @Deprecated
public static RubyBigDecimal getVpRubyObjectWithPrec19Inner(ThreadContext context, RubyRational value, RoundingMode roundingMode) {
BigDecimal numerator = BigDecimal.valueOf(RubyNumeric.num2long(value.numerator(context)));
BigDecimal denominator = BigDecimal.valueOf(RubyNumeric.num2long(value.denominator(context)));
@@ -439,6 +436,16 @@ public class RubyBigDecimal extends RubyNumeric {
return new RubyBigDecimal(context.runtime, numerator.divide(denominator, mathContext));
}
+ public static RubyBigDecimal getVpRubyObjectWithPrecRational(ThreadContext context, RubyRational value, long precision, boolean must) {
+ RubyBigDecimal numBD = getVpValueWithPrec19(context, value.numerator(context), -1, must);
+
+ if (numBD == null) {
+ return coerceErrorOrNull(context, value.numerator(context), must);
+ }
+
+ return (RubyBigDecimal) numBD.op_div19(context, value.denominator(context), context.runtime.newFixnum(precision));
+ }
+
private static RubyBigDecimal getVpValueWithPrec19(ThreadContext context, IRubyObject value, long precision, boolean must) {
if (value instanceof RubyFloat) {
if (precision > Long.MAX_VALUE) return cannotBeCoerced(context, value, must);
@@ -447,18 +454,22 @@ public class RubyBigDecimal extends RubyNumeric {
}
else if (value instanceof RubyRational) {
if (precision < 0) {
- if (must) {
- throw context.runtime.newArgumentError(value.getMetaClass().getBaseName() + " can't be coerced into BigDecimal without a precision");
- }
- return null;
+ return coerceErrorOrNull(context, value, must);
}
- return getVpRubyObjectWithPrec19Inner(context, (RubyRational) value);
+ return getVpRubyObjectWithPrecRational(context, (RubyRational) value, precision, must);
}
return getVpValue(context, value, must);
}
+ private static RubyBigDecimal coerceErrorOrNull(ThreadContext context, IRubyObject value, boolean must) {
+ if (must) {
+ throw context.runtime.newArgumentError(value.getMetaClass().getBaseName() + " can't be coerced into BigDecimal without a precision");
+ }
+ return null;
+ }
+
private static RubyBigDecimal getVpValue(ThreadContext context, IRubyObject value, boolean must) {
if (value instanceof RubyBigDecimal) return (RubyBigDecimal) value;
if (value instanceof RubyFixnum || value instanceof RubyBignum) {
@@ -1028,11 +1039,15 @@ public class RubyBigDecimal extends RubyNumeric {
// proper algorithm to set the precision
// the precision is multiple of 4
// and the precision is larger than len * 2
- int len = value.precision() + val.value.precision();
- int pow = len / 4;
- int precision = (pow + 1) * 4 * 2;
-
- return op_div(context, val, context.runtime.newFixnum(precision));
+ int aLen = value.precision() + Math.abs(value.scale());
+ int bLen = val.value.precision() + Math.abs(value.scale());
+ int max = aLen;
+ if (aLen < bLen) max = bLen;
+ max++;
+ // 38 comes from RMPD_COMPONENT_FIGURES returned by VpBaseFig in MRI
+ max = (max + 1) * 38;
+
+ return op_div(context, val, context.runtime.newFixnum(max));
}
public IRubyObject op_div(ThreadContext context, IRubyObject other) { |
|
Unfortunately this isn't going to make 9.1.6.0. I think we know the code that needs to be ported, but my port did not work right. |
|
Strangely enough, my patch no longer hangs. I'll push to a branch and see how it looks. |
|
Nevermind, it still does hang...investigating that. |
|
I was able to reproduce the hang with regular Java BigDecimal. The A script to reproduce follows. mc = java.math.MathContext.new(3800114, java.math.RoundingMode::HALF_UP)
java.math.BigDecimal.new("0.9E-99999").divide(java.math.BigDecimal.new("1E-99999"), mc)This could still be our bug, in that we're determining the precision incorrectly (or should not be passing it this way). It could also be an incompatibility between the MRI logic and Java BigDecimal. My re-porting this logic triggered the issue because it no longer implicitly caps the scale by coercing to a |
|
Ok, so made a discovery; removing the call to This isn't exactly the result we wanted (MRI appears to truncate at around 50 places) but it does tell me that this "mx" precision logic I ported is not what I thought. |
|
Ok, I think I'm getting to the bottom of this. The basic problem is that we still have a mix of 1.8 and 1.9+ coercion logic throughout our Numeric classes, sometimes we're calling the 1.8 logic incorrectly, and even the 1.9 logic we do have is in dire need of an update. In this case, a Rational passed into I think we should be able to patch up all the coercion logic by auditing all of the Numeric types. I'm not sure I'd be comfortable shipping that in 9.1.7.0 without a lot of testing. |
|
Ok, this won't happen for 9.1.7.0. As I started to pull on one thread, another came loose, and another...BigDecimal needs a re-port but that's too big to do in the next week. I will push what I have to a branch and we'll pick it up for 9.1.8.0 or 9.2. |
esp. on division/multiplication - some parts now match source closer however there's a fundamental issue in terms of MRI keeping the precision of a constructed number, while Java's BigDecimal does not ... so in the end we end up with different precision based on input. this needs to be cared out carefully as going 'too far' some of the ruby specs never finish computing (division atm) ... closes jruby#3846, jruby#4200 (we have tests guarding against regression)
esp. with division/multiplication - some parts now match C source closer however there's a fundamental difference in terms of MRI keeping the precision of a constructed number, while Java's BigDecimal doesn't ... thus we will tend to end up with different precisions based on input. this needs to be cared out carefully as going 'too far', as some of the ruby specs never finish computing (division atm) ... closes jruby#3846, jruby#4200 (we have tests guarding against regression)
esp. with division/multiplication - some parts now match C source closer however there's a fundamental difference in terms of MRI keeping the precision of a constructed number, while Java's BigDecimal doesn't ... thus we will tend to end up with different precisions based on input. this needs to be cared out carefully as going 'too far', as some of the ruby specs never finish computing (division atm) ... closes #3846, #4200 (we have tests guarding against regression)
|
this particular case is expected to be 'fixed' ... precision is still not exactly as in MRI but at least not far off |
Environment
JRuby 9.1.5.0 on OpenJDK 1.8.0_101 on x86-64
Expected Behavior
should produce (at least approximately) the same result as
which is
0.333333333333333333336666666666666666666666666667E0.Actual Behavior
actually produces result
0.333300000000000000003333E0.The effective precision is merely 4 decimal digits, far worse than if Float was used.
Additional comments
The requested precision should be at least the precision of the left operand, if not substantially more (e.g. twice the precision of the left operand).
The text was updated successfully, but these errors were encountered: