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::limit has not effect #1615

Closed
nmk opened this Issue Apr 9, 2014 · 10 comments

Comments

Projects
None yet
7 participants
@nmk

nmk commented Apr 9, 2014

Using BigDecimal::limit has not effect on following computations with BigDecimal instances. This works in MRI 1.9.3 and later.

Compare the following two sessions:

MRI (1.9.3-p545):

~ $ irb -r"bigdecimal" -r "bigdecimal/util"
1.9.3-p545 :001 > BigDecimal.limit(3)
 => 0
1.9.3-p545 :002 > 10.to_d / 3.to_d
 => #<BigDecimal:7fd034801d38,'0.333E1',18(36)>
1.9.3-p545 :003 > (10.to_d / 3.to_d).to_f
 => 3.33
1.9.3-p545 :004 > (10.to_d / 3.to_d).to_s("F")
 => "3.33"  

Latest JRuby (1.7.11):

~ $ irb -r"bigdecimal" -r"bigdecimal/util"
jruby-1.7.11 :001 > BigDecimal.limit(3)
 => 0
jruby-1.7.11 :002 > 10.to_d / 3.to_d
 => #<BigDecimal:136c2147,'0.33333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333E1',200(204)>
jruby-1.7.11 :003 > (10.to_d / 3.to_d).to_f
 => 3.3333333333333335
jruby-1.7.11 :004 > (10.to_d / 3.to_d).to_s("F")
 => "3.3333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333"
@masover

This comment has been minimized.

Show comment
Hide comment
@masover

masover Apr 29, 2014

I can reproduce this in 1.7.12.

masover commented Apr 29, 2014

I can reproduce this in 1.7.12.

@masover

This comment has been minimized.

Show comment
Hide comment
@masover

masover Apr 30, 2014

I can reproduce it as recently as 5ee515f.

The spec that probably should catch this is in spec/ruby/library/bigdecimal/limit_spec.rb. It currently checks that rounding is applied correctly on addition and multiplication, but doesn't test division. (And addition and multiplication do follow the global limit setting.) And division works fine with a local limit:

$ irb -r"bigdecimal" -r"bigdecimal/util"
irb(main):001:0> 10.to_d.div(3.to_d, 3)
=> #<BigDecimal:1bb82408,'0.333E1',3(4)>
irb(main):002:0> 10.to_d.div(3.to_d, 3).to_f
=> 3.33

So I'm optimistic about this one.

masover commented Apr 30, 2014

I can reproduce it as recently as 5ee515f.

The spec that probably should catch this is in spec/ruby/library/bigdecimal/limit_spec.rb. It currently checks that rounding is applied correctly on addition and multiplication, but doesn't test division. (And addition and multiplication do follow the global limit setting.) And division works fine with a local limit:

$ irb -r"bigdecimal" -r"bigdecimal/util"
irb(main):001:0> 10.to_d.div(3.to_d, 3)
=> #<BigDecimal:1bb82408,'0.333E1',3(4)>
irb(main):002:0> 10.to_d.div(3.to_d, 3).to_f
=> 3.33

So I'm optimistic about this one.

@dummey

This comment has been minimized.

Show comment
Hide comment
@dummey

dummey May 3, 2014

https://github.com/dummey/jruby/blob/cd1677eefe3fd79d9ea69660ab3f781457789311/spec/ruby/library/bigdecimal/limit_spec.rb

Adding some checks for subtraction and division for the fun of it.

Edit: https://github.com/dummey/jruby/blob/51a4ae12719a6b6491e403917f3976c061e38d23/spec/ruby/library/bigdecimal/limit_spec.rb

Divided up the spec to make it easier to see what is failing. It looks like subtraction is fine, it's just division that is not adhering to the limit.

dummey commented May 3, 2014

https://github.com/dummey/jruby/blob/cd1677eefe3fd79d9ea69660ab3f781457789311/spec/ruby/library/bigdecimal/limit_spec.rb

Adding some checks for subtraction and division for the fun of it.

Edit: https://github.com/dummey/jruby/blob/51a4ae12719a6b6491e403917f3976c061e38d23/spec/ruby/library/bigdecimal/limit_spec.rb

Divided up the spec to make it easier to see what is failing. It looks like subtraction is fine, it's just division that is not adhering to the limit.

@dummey

This comment has been minimized.

Show comment
Hide comment
@dummey

dummey May 3, 2014

Interesting...

// regular division with some default precision
// TODO: proper algorithm to set the precision

From: jruby/core/src/main/java/org/jruby/ext/bigdecimal/RubyBigDecimal.java:[861]

dummey commented May 3, 2014

Interesting...

// regular division with some default precision
// TODO: proper algorithm to set the precision

From: jruby/core/src/main/java/org/jruby/ext/bigdecimal/RubyBigDecimal.java:[861]

@masover

This comment has been minimized.

Show comment
Hide comment
@masover

masover May 3, 2014

I actually have something I could submit, but the spec (yours or mine) should probably go in RubySpec first?

And, that's exactly where I ended up. I still don't have a proper algorithm, but it's not difficult to at least enforce a limit (if one is provided).

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 383747a..309aaae 100644
--- a/core/src/main/java/org/jruby/ext/bigdecimal/RubyBigDecimal.java
+++ b/core/src/main/java/org/jruby/ext/bigdecimal/RubyBigDecimal.java
@@ -856,7 +856,12 @@ public class RubyBigDecimal extends RubyNumeric {
         RubyObject preciseOther = getVpValue19(context, other, true);
         // regular division with some default precision
         // TODO: proper algorithm to set the precision
-        return op_div(context, preciseOther, getRuntime().newFixnum(200));
+        Ruby runtime = getRuntime();
+        IRubyObject lim = bigDecimal(runtime).searchInternalModuleVariable("vpPrecLimit");
+        if (lim.equals(RubyFixnum.zero(runtime))) {
+            lim = getRuntime().newFixnum(200);
+        }
+        return op_div(context, preciseOther, lim);
     }

     public IRubyObject op_div(ThreadContext context, IRubyObject other) {

If you can wait, I can build a proper pull request. If you want to take my patch and run with it, I won't be offended.

masover commented May 3, 2014

I actually have something I could submit, but the spec (yours or mine) should probably go in RubySpec first?

And, that's exactly where I ended up. I still don't have a proper algorithm, but it's not difficult to at least enforce a limit (if one is provided).

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 383747a..309aaae 100644
--- a/core/src/main/java/org/jruby/ext/bigdecimal/RubyBigDecimal.java
+++ b/core/src/main/java/org/jruby/ext/bigdecimal/RubyBigDecimal.java
@@ -856,7 +856,12 @@ public class RubyBigDecimal extends RubyNumeric {
         RubyObject preciseOther = getVpValue19(context, other, true);
         // regular division with some default precision
         // TODO: proper algorithm to set the precision
-        return op_div(context, preciseOther, getRuntime().newFixnum(200));
+        Ruby runtime = getRuntime();
+        IRubyObject lim = bigDecimal(runtime).searchInternalModuleVariable("vpPrecLimit");
+        if (lim.equals(RubyFixnum.zero(runtime))) {
+            lim = getRuntime().newFixnum(200);
+        }
+        return op_div(context, preciseOther, lim);
     }

     public IRubyObject op_div(ThreadContext context, IRubyObject other) {

If you can wait, I can build a proper pull request. If you want to take my patch and run with it, I won't be offended.

@dummey

This comment has been minimized.

Show comment
Hide comment
@dummey

dummey May 3, 2014

You should go for it =D

I'm exploring and tagging along largely to learn more about the inner workings of JRuby, so this would be a great chance for me to 'stalk' what you do.

dummey commented May 3, 2014

You should go for it =D

I'm exploring and tagging along largely to learn more about the inner workings of JRuby, so this would be a great chance for me to 'stalk' what you do.

@dymaxionuk

This comment has been minimized.

Show comment
Hide comment
@dymaxionuk

dymaxionuk Jul 22, 2016

This bug has been open for 2 years. Surprised nobody else has felt the pain!

Since #limit is not working for division, I have to post process all my output data, and traverse every field in my data structures and manually round.

This is cumbersome. Can someone help fix this? Thanks

dymaxionuk commented Jul 22, 2016

This bug has been open for 2 years. Surprised nobody else has felt the pain!

Since #limit is not working for division, I have to post process all my output data, and traverse every field in my data structures and manually round.

This is cumbersome. Can someone help fix this? Thanks

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Jul 22, 2016

Member

@dymaxionuk you could have tried fixing it than or posting a bounty. it shouldn't be very hard and there's even some code in the comments to start with ...

Member

kares commented Jul 22, 2016

@dymaxionuk you could have tried fixing it than or posting a bounty. it shouldn't be very hard and there's even some code in the comments to start with ...

@MrBerg

This comment has been minimized.

Show comment
Hide comment
@MrBerg

MrBerg Oct 27, 2017

Contributor

I made a pull request for this: #4830

Contributor

MrBerg commented Oct 27, 2017

I made a pull request for this: #4830

headius added a commit that referenced this issue Oct 30, 2017

Merge pull request #4830 from MrBerg/enforce-bigdecimal-limit
Make BigDecimal::limit work for subtraction and division. Fix issue #1615

@headius headius added this to the JRuby 9.1.14.0 milestone Oct 30, 2017

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 30, 2017

Member

Fixed in 9.1.14 and 9.2. See #4830 for details.

Member

headius commented Oct 30, 2017

Fixed in 9.1.14 and 9.2. See #4830 for details.

@headius headius closed this Oct 30, 2017

eregon added a commit that referenced this issue Dec 1, 2017

Squashed 'spec/ruby/' changes from a6b8805..bacedc5
bacedc5 Exclude a couple specs not supported on Windows
7a2c8be Disable Lint/AmbiguousBlockAssociation
677c103 Fix indentation
8eb3a31 Fix trailing whitespace
b8d6e05 Suppress warnings around intended constant re-definition
0e31c13 Add RUBYOPT with extra whitespace command line spec
84e28fc Restrict the File.utime spec with large times to Linux 64-bit
47b9727 Add spec for File.utime with Time instances far in the future
421b858 Make sure to read the output of date in Kernel#open specs
baf5d11 Add spec for IO#sysread
1884b1d Add specs that IO#{read,write}_nonblock sets the IO to nonblock mode
4d60f75 Improve Time#round spec
77ba816 Improve thread local specs
240afea Add FL_ABLE and FL_TEST specs
c5bf527 Organize the Process.groups= spec
ba8700e Move the Process.groups= example under the Process.groups= spec
fb9c3a2 Fix DateTime.now spec
05b3ca1 Fix expectation in Dir#chroot spec
2ff8dc3 Add `rb_scan_args' specs for optional kwargs.
610a22f Give more margin for Mutex#sleep sleep time
3a7d4e1 Use a monotonic time to measure real-time intervals in Mutex#sleep spec
b6e9294 Specs for string issues found. Yaml test currently tagged.
de9ecda Implement rb_absint_singlebit_p and add spec
250e29a Only require fixtures needing refinements in the examples that need it
6b05eed Avoid loading a fixture using #refine while loading
c7c888a Fix IO#each spec
4cda555 Add spec for Time.{utc,gm} with small fractional usec.
12ab462 Add spec for tiny Mutex#sleep times.
6b14dca Spec for __FILE__ in BEGIN. See jruby/jruby#4677.
3a9d651 Add spec for looped delegation of a block containing a break.
d04818f Added specs for IO#each(sep,limit) (jruby/jruby#4833)
df0a78f Add spec for Time#to_datetime
2e1c77c Add a spec for DateTime#to_time
7e69e0a Basic constant assign specs for &&=
9089561 Make BigDecimal::limit work for subtraction and division. Fix jruby/jruby#1615.
3041df8 Adapt sprintf/format specs to handle the 2.5 behavior for sprintf("%")
4c29047 Restore the return in class spec and specify the old and new behaviors
1da2fca Put a problematic refine spec in quarantine
7db8544 spec/ruby/optional/capi/constants_spec.rb: Data is deprecated now
85f85b4 * append newline at EOF.
ec75aa8 Add specs for concurrent Module#autoload
0fcde61 Reorganize Module#autoload to have similar specs next to each other
eef31f7 parse.y: no return in class
7f8a8b7 load from relative path to __FILE__
85562a8 Clarify what is written and read in IO#popen spec
cc7bfbd Fix spec which can fail if the pipe is closed before flushing in the subprocess
9f8d3d0 Some methods of Module are now public
f2f23ad Fix File#printf specs running on Windows
bcc750f Suppress Rubocop warnings
bc797f8 Code review. Share test cases for b and B, g and G, e and E, d, i and u formats
91ab65a Code review. Minor issues
e1d646d Remove excessive specs
b8bbe86 Adopt already existed test cases
5a30b08 Include shared example for StringIO#printf
8de650e Include shared example for String#%
19ce86a Include shared example for File#printf
b77658e Include shared example for Kernel#sprintf
1ddb415 Include shared example for Kernel#printf
764b78b Shared example: Add reference by name
acc3bdf Shared example: Add precision
b001a62 Shared example: Add width
1800767 Shared example: Add flags
f148cc7 Shared example: Add types
760a41e Add specs for Array#append [Feature #12746]
8bde28c Move Array#push specs to shared/
bc769a8 Remove superfluous spec
e32f79f Add specs for Array#prepend [Feature #12746]
ff2eb10 Move Array#unshift specs to shared/
e7ac1ca Test that the rescue in method arguments fails when not between parens
a7770da Update line number
43fe574 Remove old guard
e7598e6 Remove guards for old versions
f3599a7 Clarify which part should be undefined in defined? specs
76df3f7 Avoid defining top-level constants in TracePoint specs
e84c928 #513 Add TracePoint specs (#518)
7d76b72 Workaround Travis CI issue by hardcoding localhost
2025413 fix trailing space
f179d05 Avoid plain "should raise_error" by specifying an expected error class
6a2034c Require spec_helper should be first
2ab4d54 Pick up sibling mspec when running a spec with 'ruby' directly
214791f Fix File::Stat#blocks spec to allow zero
11a0cf2 Fix platform dependent specs for File.empty?
663d8eb Ignore the libruby check if it cannot be found
7069967 Make sure to compile each extension only once in ruby/spec
c051e55 Use ENV['RUBY_EXE'] as RbConfig.ruby might be incorrect
6b65a12 * append newline at EOF.

git-subtree-dir: spec/ruby
git-subtree-split: bacedc542c85b825f01d2698d037fad8e1c012a4

eregon added a commit to oracle/truffleruby that referenced this issue Dec 1, 2017

Squashed 'spec/ruby/' changes from 663d8eb..bacedc5
bacedc5 Exclude a couple specs not supported on Windows
7a2c8be Disable Lint/AmbiguousBlockAssociation
677c103 Fix indentation
8eb3a31 Fix trailing whitespace
b8d6e05 Suppress warnings around intended constant re-definition
0e31c13 Add RUBYOPT with extra whitespace command line spec
84e28fc Restrict the File.utime spec with large times to Linux 64-bit
47b9727 Add spec for File.utime with Time instances far in the future
421b858 Make sure to read the output of date in Kernel#open specs
baf5d11 Add spec for IO#sysread
1884b1d Add specs that IO#{read,write}_nonblock sets the IO to nonblock mode
4d60f75 Improve Time#round spec
77ba816 Improve thread local specs
240afea Add FL_ABLE and FL_TEST specs
c5bf527 Organize the Process.groups= spec
ba8700e Move the Process.groups= example under the Process.groups= spec
fb9c3a2 Fix DateTime.now spec
05b3ca1 Fix expectation in Dir#chroot spec
2ff8dc3 Add `rb_scan_args' specs for optional kwargs.
610a22f Give more margin for Mutex#sleep sleep time
3a7d4e1 Use a monotonic time to measure real-time intervals in Mutex#sleep spec
b6e9294 Specs for string issues found. Yaml test currently tagged.
de9ecda Implement rb_absint_singlebit_p and add spec
250e29a Only require fixtures needing refinements in the examples that need it
6b05eed Avoid loading a fixture using #refine while loading
c7c888a Fix IO#each spec
4cda555 Add spec for Time.{utc,gm} with small fractional usec.
12ab462 Add spec for tiny Mutex#sleep times.
6b14dca Spec for __FILE__ in BEGIN. See jruby/jruby#4677.
3a9d651 Add spec for looped delegation of a block containing a break.
d04818f Added specs for IO#each(sep,limit) (jruby/jruby#4833)
df0a78f Add spec for Time#to_datetime
2e1c77c Add a spec for DateTime#to_time
7e69e0a Basic constant assign specs for &&=
9089561 Make BigDecimal::limit work for subtraction and division. Fix jruby/jruby#1615.
3041df8 Adapt sprintf/format specs to handle the 2.5 behavior for sprintf("%")
4c29047 Restore the return in class spec and specify the old and new behaviors
1da2fca Put a problematic refine spec in quarantine
7db8544 spec/ruby/optional/capi/constants_spec.rb: Data is deprecated now
85f85b4 * append newline at EOF.
ec75aa8 Add specs for concurrent Module#autoload
0fcde61 Reorganize Module#autoload to have similar specs next to each other
eef31f7 parse.y: no return in class
7f8a8b7 load from relative path to __FILE__
85562a8 Clarify what is written and read in IO#popen spec
cc7bfbd Fix spec which can fail if the pipe is closed before flushing in the subprocess
9f8d3d0 Some methods of Module are now public
f2f23ad Fix File#printf specs running on Windows
bcc750f Suppress Rubocop warnings
bc797f8 Code review. Share test cases for b and B, g and G, e and E, d, i and u formats
91ab65a Code review. Minor issues
e1d646d Remove excessive specs
b8bbe86 Adopt already existed test cases
5a30b08 Include shared example for StringIO#printf
8de650e Include shared example for String#%
19ce86a Include shared example for File#printf
b77658e Include shared example for Kernel#sprintf
1ddb415 Include shared example for Kernel#printf
764b78b Shared example: Add reference by name
acc3bdf Shared example: Add precision
b001a62 Shared example: Add width
1800767 Shared example: Add flags
f148cc7 Shared example: Add types
760a41e Add specs for Array#append [Feature #12746]
8bde28c Move Array#push specs to shared/
bc769a8 Remove superfluous spec
e32f79f Add specs for Array#prepend [Feature #12746]
ff2eb10 Move Array#unshift specs to shared/
e7ac1ca Test that the rescue in method arguments fails when not between parens
a7770da Update line number
43fe574 Remove old guard
e7598e6 Remove guards for old versions
f3599a7 Clarify which part should be undefined in defined? specs
76df3f7 Avoid defining top-level constants in TracePoint specs
e84c928 #513 Add TracePoint specs (#518)
7d76b72 Workaround Travis CI issue by hardcoding localhost
2025413 fix trailing space
f179d05 Avoid plain "should raise_error" by specifying an expected error class
6a2034c Require spec_helper should be first
2ab4d54 Pick up sibling mspec when running a spec with 'ruby' directly
214791f Fix File::Stat#blocks spec to allow zero
11a0cf2 Fix platform dependent specs for File.empty?

git-subtree-dir: spec/ruby
git-subtree-split: bacedc542c85b825f01d2698d037fad8e1c012a4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment