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

BigDecimal/float difference between MRI and JRuby #5904

Open
Confusion opened this issue Oct 3, 2019 · 5 comments
Open

BigDecimal/float difference between MRI and JRuby #5904

Confusion opened this issue Oct 3, 2019 · 5 comments

Comments

@Confusion
Copy link

Expected Behavior

In MRI the following comparison returns false:

2.5.3 :001 > BigDecimal.new('.803e1') == 8.03
 => false 

This is probably because there is no exact floating point representation for the number 8.03 (as opposed to e.g. 8.02 and 8.04, for which the similar comparison returns true.

Actual Behavior

jruby-9.2.8.0 :001 > BigDecimal.new('0.803e1') == 8.03
 => true 

This is not a problem for me, except that it was surprising that a spec passed in JRuby that I expected to fail.

Environment

  • jruby 9.2.8.0 (2.5.3) 2019-08-20 a1ac7ff OpenJDK 64-Bit Server VM 11.0.4+11-post-Ubuntu-1ubuntu218.04.3 on 11.0.4+11-post-Ubuntu-1ubuntu218.04.3 +jit [linux-x86_64]
  • Linux photon 4.15.0-58-generic preventing stale open SocketChannels #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
@mohits
Copy link

mohits commented May 8, 2021

I think that this issue should be closed since this is probably implementation dependent, and should not be something that a programmer should rely on.

@headius
Copy link
Member

headius commented May 10, 2021

Looking into this a little big (apparently for the first time)...

I believe this is an artifact of how we compare a BigDecimal with a Float. In JRuby, we promote the Float into a BigDecimal by using the java.math.BigDecimal constructor that takes a java.math.MathContext for precision. Specifically here in the code, where we use a precision of 15 for floats but unlimited for integer values:

private static RubyBigDecimal getVpValue(ThreadContext context, IRubyObject value, boolean must) {
switch (((RubyBasicObject) value).getNativeClassIndex()) {
case BIGDECIMAL:
return (RubyBigDecimal) value;
case FIXNUM:
return newInstance(context.runtime, context.runtime.getClass("BigDecimal"), (RubyFixnum) value, MathContext.UNLIMITED);
case BIGNUM:
return newInstance(context.runtime, context.runtime.getClass("BigDecimal"), (RubyBignum) value, MathContext.UNLIMITED);
case FLOAT:
return newInstance(context.runtime, context.runtime.getClass("BigDecimal"), (RubyFloat) value, new MathContext(RubyFloat.DIG));
case RATIONAL:
return newInstance(context, (RubyRational) value, new MathContext(RubyFloat.DIG));
}
return cannotBeCoerced(context, value, must);
}

Duplicating this code from Ruby we see the same result:

$ jruby -e "p java.math.BigDecimal.new('0.803e1').compareTo(java.math.BigDecimal.new(8.03, java.math.MathContext.new(15)))"
0

Leaving the precision off gets us a result that matches CRuby:

$ rvm ruby-3.0 do ruby -rbigdecimal -e "p BigDecimal('0.803e1') <=> 8.03"
1

$ jruby -e "p java.math.BigDecimal.new('0.803e1').compareTo(java.math.BigDecimal.new(8.03))"
1

As near as I can tell this change came in 73764ea as part of improving the coercion process for Float values. I am not sure where the 15 precision came from, but if we revisit CRuby's equivalent to getVpValue we may be able to match their behavior more closely.

@headius
Copy link
Member

headius commented May 10, 2021

The CRuby GetVpValue code basically converts the Float value into a string, then normalizes that string into an Integer and an exponent. The logic bottoms out around here:

https://github.com/ruby/ruby/blob/4e2e1d60936a3dcbc0e51b1c9c925ef41c1780f6/ext/bigdecimal/bigdecimal.c#L2960

I believe this means it will end up using infinite precision based on the normalized Integer form of the Float which would explain the behavioral difference from our 15 precision logic.

As of Java 8, the java.math.BigDecimal logic does not convert the incoming double value into a String. Instead it works with the raw bits directly and pulls out each element of the floating-point value to use in constructing the BigDecimal instance. The value is then rounded to the requested precision, which results in the exact BigDecimal 8.03 actually match the inaccurate floating point 8.029999999999999-something.

So basically, by providing the precision and allowing it to round off, we are choosing one inaccurate representation over another, and end up matching.

@ahorek I believe this was your commit. I know it has been a while, but do you remember why you chose to pass in a MathContext with a precision here?

The fix for this would be to figure out what precision and rounding behavior matches CRuby's float-to-string-to-integer path.

@ahorek
Copy link
Contributor

ahorek commented May 11, 2021

actually 73764ea didn't cause the regression.

why you chose to pass in a MathContext with a precision here?

the idea behind this is that more accurate precision than the maximum float precision doesn't matter for a comparsion with another float.

the flaw is we're not comparing the real float representation which is inacurrate.

BigDecimal('.803e1') == 8.03

[BigDecimal('.803e1'), BigDecimal('.803e1')]
=> true # current behaviour

[BigDecimal(8.02999973297119140625), BigDecimal('.803e1')]
=> false # desired behaviour

so the value should be normalized before converting to a bigdecimal. The precision isn't a real problem.

also the way how CRuby do it seems to be complex and probably slow. The best way would be to use the fast path which works for most cases and do an additional validation step if there's a match for corner cases like this.

btw: I don't see a real use case why should anyone depend on precise comparsions between floats and bigdecimals.
But yes, it's a bug - BigDecimal('8.03') and Float(8.03) aren't the same numbers even if they look very simmilar.

@headius
Copy link
Member

headius commented May 13, 2021

the flaw is we're not comparing the real float representation which is inacurrate.

You are correct. I did not mean to imply that you broke anything. It is largely a bug in us following CRuby logic for the comparison but not for the "GetVpValue" logic that converts float to BigDecimal via a string.

I think we will need to diverge on the implementation of RubyBigDecimal.cmp in order to do more accurate comparisons. Truth be told, we could probably drop most of the ported logic from CRuby if we can get the JDK BigDecimal to do the equivalent work.

At this point I am not sure what the fix should be, or indeed whether a fix is necessary. These values are logically equal but physically unequal, and every resource I have looked at recommends against comparing floating-point values and BigDecimal values without explicitly opting into the rounding. In this case we are rounding to 15 digits which causes them to appear equal, while 16 would cause them to be unequal:

$ jruby -rbigdecimal -e "p BigDecimal(8.03, 15)"
0.803e1

$ jruby -rbigdecimal -e "p BigDecimal(8.03, 16)"
0.8029999999999999e1

Even if we were to skip this coercion to BigDecimal at a Ruby level, we would still need to coerce to a JDK BigDecimal in order to do the comparison, at which point we would need to choose some level of rounding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants