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

BigDecimal multiplication with Rational produces garbage digits within the requested precision. #4200

Closed
felixvf opened this Issue Oct 3, 2016 · 11 comments

Comments

Projects
None yet
3 participants
@felixvf

felixvf commented Oct 3, 2016

# ruby -e 'require "bigdecimal"; require "rational"; puts BigDecimal.new("1.00000000000000000001")/3; puts BigDecimal.new("1.00000000000000000001")*Rational(1,3)'
0.333333333333333333336666666666666666666666666667E0
0.333300000000000000003333E0

Environment

JRuby 9.1.5.0 on OpenJDK 1.8.0_101 on x86-64

Expected Behavior

BigDecimal.new("1.00000000000000000001")*Rational(1,3)

should produce (at least approximately) the same result as

BigDecimal.new("1.00000000000000000001")/3

which is 0.333333333333333333336666666666666666666666666667E0.

Actual Behavior

BigDecimal.new("1.00000000000000000001")*Rational(1,3)

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

@headius

This comment has been minimized.

Member

headius commented Oct 6, 2016

Yow, that's weird. I guess we're coercing the Rational to a low-precision float or something. Very strange.

@headius headius added this to the JRuby 9.1.6.0 milestone Oct 6, 2016

@headius

This comment has been minimized.

Member

headius commented Oct 7, 2016

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:

$ jruby -rbigdecimal -e 'p BigDecimal.new("1.00000000000000000001")*Rational(1,3)'
#<BigDecimal:77f99a05,'0.33333333333333333333666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666633333333333333333333E0',134(136)>

I'll keep poking at it a bit this morning and at least post a patch.

@headius

This comment has been minimized.

Member

headius commented Oct 7, 2016

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: BigDecimal("0.9E-99999").div(BigDecimal("1E-99999"), 0)

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) {
@headius

This comment has been minimized.

Member

headius commented Nov 8, 2016

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.

@headius headius modified the milestones: JRuby 9.1.7.0, JRuby 9.1.6.0 Nov 8, 2016

@headius

This comment has been minimized.

Member

headius commented Dec 13, 2016

Strangely enough, my patch no longer hangs. I'll push to a branch and see how it looks.

@headius

This comment has been minimized.

Member

headius commented Dec 13, 2016

Nevermind, it still does hang...investigating that.

@headius

This comment has been minimized.

Member

headius commented Dec 13, 2016

I was able to reproduce the hang with regular Java BigDecimal. The div logic in Ruby attempts to determine an upper scale when none is provided. In this case, that value ends up to be 3800114, and it appears that the Java BigDecimal logic is simply running forever trying to fill in that many significant figures.

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 long (which is what this patch was intended to support).

@headius

This comment has been minimized.

Member

headius commented Dec 14, 2016

Ok, so made a discovery; removing the call to quo and the autoscaling of the precision makes this work correctly, and with a significantly longer tail of digits than before:

$ jruby -rbigdecimal -e 'puts BigDecimal.new("1.00000000000000000001")/3; puts BigDecimal.new("1.00000000000000000001")*Rational(1,3)'
0.33333333333333333333666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666667E0
Unhandled Java exception: java.lang.ArithmeticException: Non-terminating decimal expansion; no exact representable decimal result.
java.lang.ArithmeticException: Non-terminating decimal expansion; no exact representable decimal result.
                           divide at java/math/BigDecimal.java:1690
                           divide at java/math/BigDecimal.java:1723
                           op_div at org/jruby/ext/bigdecimal/RubyBigDecimal.java:1107
                         op_div19 at org/jruby/ext/bigdecimal/RubyBigDecimal.java:1119
  getVpRubyObjectWithPrecRational at org/jruby/ext/bigdecimal/RubyBigDecimal.java:446
             getVpValueWithPrec19 at org/jruby/ext/bigdecimal/RubyBigDecimal.java:460
                     getVpValue19 at org/jruby/ext/bigdecimal/RubyBigDecimal.java:424
                          mult219 at org/jruby/ext/bigdecimal/RubyBigDecimal.java:765
                         op_mul19 at org/jruby/ext/bigdecimal/RubyBigDecimal.java:756
                             call at org/jruby/ext/bigdecimal/RubyBigDecimal$INVOKER$i$1$0$op_mul19.gen:-1
                     cacheAndCall at org/jruby/runtime/callsite/CachingCallSite.java:338
                             call at org/jruby/runtime/callsite/CachingCallSite.java:163
                   invokeOther8:* at -e:1
                           <main> at -e:1

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.

@headius

This comment has been minimized.

Member

headius commented Dec 14, 2016

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 * should be coerced to num/den with a precision derived from the LHS. Our current logic called 1.8 coercion paths that error hard and refuse to coerce.

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.

@headius

This comment has been minimized.

Member

headius commented Dec 14, 2016

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.

@headius headius modified the milestones: JRuby 9.1.8.0, JRuby 9.1.7.0 Dec 14, 2016

@headius headius modified the milestones: JRuby 9.2.0.0, JRuby 9.1.8.0 Mar 3, 2017

@kares kares self-assigned this Apr 14, 2018

kares added a commit to kares/jruby that referenced this issue Apr 15, 2018

let's try 'better' approximating MRI's BigDecimal in terms on precision
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)

kares added a commit to kares/jruby that referenced this issue Apr 15, 2018

let's try 'better' approximating MRI's BigDecimal precision (/ and *)
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)

kares added a commit that referenced this issue Apr 15, 2018

let's try 'better' approximating MRI's BigDecimal precision (/ and *)
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)
@kares

This comment has been minimized.

Member

kares commented Apr 16, 2018

this particular case is expected to be 'fixed' ... precision is still not exactly as in MRI but at least not far off
... a test based on report is also there: 4b004f1#diff-7d7b709cf82c505e6c22ca0773282e2cR410

@kares kares closed this Apr 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment