Backport #3803

BigDecimal::ROUND_HALF_DOWN/ROUND_HALF_EVEN behave incorrectly (disagree with JRuby and the rest of the world)

Added by Matthew Willson over 3 years ago. Updated over 2 years ago.

[ruby-core:32136]
Status:Closed
Priority:Normal
Assignee:Yuki Sonoda

Description

=begin
The incorrect behaviour is that all fractional values between 0.5 (inclusive) and 0.6 (non-inclusive) are subject to the rounding policy for 'half', whereas it should only be applied for fractional values exactly equal to 0.5.

This means that, for example, 0.59 is rounded down to 0 by ROUNDHALFDOWN and ROUNDHALFEVEN:

 >> BigDecimal.new("0.59").round(0, BigDecimal::ROUND_HALF_DOWN).to_s('F')
 => "0.0"
 >> BigDecimal.new("0.56").round(0, BigDecimal::ROUND_HALF_EVEN).to_s('F')
 => "0.0"

The behaviour is actually specified this way in the RDoc:

ROUNDHALFDOWN: round up if the appropriate digit >= 6, otherwise truncate

But this is most definitely not what most people would expect from 'round half down' (see eg: http://en.wikipedia.org/wiki/Rounding#Round_half_down ). I would expect it to only round down if the fractional part is less than or exactly equal to 0.5. It needs to take into account more than just the first decimal digit to do this; while the first decimal digit of 0.59 may be 5, it is not equal to 0.5 and hence should not be subject to the 'half down' rounding policy. (Note that ROUNDHALFUP still works correctly, since the correct behaviour here can indeed be implemented by only inspecting the first decimal digit; but this is not the case for ROUNDHALFDOWN or in general for ROUNDHALFEVEN)

Rather misleadingly, ROUNDHALFEVEN is specified as being "banker's rounding":

ROUNDHALFEVEN: round towards the even neighbor (Banker’s rounding)

But since it incorrectly applies the 'half towards even' policy not just to 0.5, but to all values in the [0.5, 0.6) interval, it is not "banker's rounding" as this is usually defined, and is surely a biased rounding policy, which is exactly what banker's rounding is seeking to avoid (see eg http://en.wikipedia.org/wiki/Rounding#Round_half_to_even )

I note (as just one example) that Java's BigDecimal specifies and implements ROUNDHALFDOWN in the correct way: http://download-llnw.oracle.com/javase/6/docs/api/java/math/BigDecimal.html#ROUND_HALF_DOWN

Rounding mode to round towards "nearest neighbor" unless both neighbors are equidistant, in which case round down.

And that JRuby's BigDecimal appears to be based on this, hence is correct and hence disagrees with MRI when (for example) applying ROUNDHALFDOWN to 0.59:

 irb(main):004:0> RUBY_PLATFORM
 => "java"
 irb(main):005:0> BigDecimal.new("0.59").round(0, BigDecimal::ROUND_HALF_DOWN).to_s('F')
 => "1.0"

Backport to 1.8.7 would be nice if this gets fixed.
=end

ruby_bigdecimal_round_half.patch Magnifier - Minimal patch with test and docfix (3.88 KB) Matthew Willson, 09/15/2010 10:56 AM

ruby_bigdecimal_round_half_with_comments.patch Magnifier - Patch with some extra comments which helped me figure out what's going on (8.35 KB) Matthew Willson, 09/15/2010 10:56 AM


Related issues

Duplicated by ruby-trunk - Bug #4567: BigDecimal::ROUND_HALF_DOWN Closed 04/11/2011

Associated revisions

Revision 29293
Added by Kenta Murata over 3 years ago

  • ext/bigdecimal/bigdecimal.c: fix rounding algorithms for half-down and half-even. This change is based on the patch created by Matthew Willson, the reporter of this bug. [Bug #3803]
  • test/bigdecimal/test_bigdecimal.rb: add tests for above changes.

History

#1 Updated by Shyouhei Urabe over 3 years ago

  • Status changed from Open to Assigned
  • Assignee set to Kenta Murata

=begin

=end

#2 Updated by Kenta Murata over 3 years ago

  • Category set to ext
  • Target version set to 1.9.3

=begin
This is bugs of BigDecimal, which have not been noticed for a long time.
Thank you very much for your report.

While I will correct these bugs in a few days,
if you have appropriate patches, I want to review and incorporate them.

=end

#4 Updated by Matthew Willson over 3 years ago

=begin
Thanks Kenta, yeah that would be great if you get a moment to review the attached patch. I included a regression test and a fix to the rdoc comments. It fixes the issue for me.

The version with comments includes some comments I added to help me figure out what's going on there, the other patch is more minimal.
=end

#5 Updated by Matthew Willson over 3 years ago

=begin
(I should say the patch is against SVN trunk)
=end

#6 Updated by Kenta Murata over 3 years ago

=begin
Thank you for attaching your patches.
I'll review them tonight.

=end

#7 Updated by Kenta Murata over 3 years ago

  • Status changed from Assigned to Closed
  • % Done changed from 0 to 100

=begin
This issue was solved with changeset r29293.
Matthew, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

=end

#8 Updated by Shane Emmons about 3 years ago

=begin
Why isn't this being back-ported into 1.8.7 and 1.9.2? This is a rather critical bug to anyone working with financial transactions. It seems like this is an obvious bug that should be fixed in all currently maintained branches.
=end

#9 Updated by Kenta Murata about 3 years ago

  • Tracker changed from Bug to Backport
  • Status changed from Closed to Assigned
  • Assignee changed from Kenta Murata to Yuki Sonoda

=begin

=end

#10 Updated by Kenta Murata about 3 years ago

  • Target version changed from 1.9.3 to 1.9.2

=begin

=end

#11 Updated by Kenta Murata over 2 years ago

  • Status changed from Assigned to Closed

This issue was solved with changeset r29293.
Matthew, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


  • ext/bigdecimal/bigdecimal.c: fix rounding algorithms for half-down and half-even. This change is based on the patch created by Matthew Willson, the reporter of this bug. [Bug #3803]
  • test/bigdecimal/test_bigdecimal.rb: add tests for above changes.

#12 Updated by Yui NARUSE over 2 years ago

  • Target version deleted (1.9.2)

Also available in: Atom PDF