Project

General

Profile

Bug #12548

Rounding modes inconsistency between round versus sprintf

Added by unclekiki (Kieran McCusker) over 1 year ago. Updated 10 months ago.

Status:
Open
Priority:
Normal
Assignee:
-
Target version:
-
[ruby-core:76253]

Description

Hi

I've found this possible bug with sprintf in our production code, but it
seems very odd that no one has reported it so I'm doubting myself.

I've tested a few versions (CRuby 2.3.1 and 2.1.9) and it was present in
both.

To reproduce in irb:

sprintf('%1.0f', 12.5)

Expected result 13 actual 12

In fact if you look at the number sequence 0.5 -> 99.5 you find the even
half will all round down and the odd half round up. 12.5.round produces the correct result (13)

If you do the same in jruby 2.2.1 you get the expected result of 13.

Many thanks

Kieran

round_even.patch (4.43 KB) round_even.patch noahgibbs (Noah Gibbs), 09/22/2016 04:40 AM

Related issues

Related to Ruby trunk - Bug #12958: Breaking change in how `#round` worksClosed

Associated revisions

Revision 56590
Added by nobu (Nobuyoshi Nakada) 12 months ago

numeric.c: round to nearest even

  • numeric.c (flo_round, int_round): support round-to-nearest-even semantics of IEEE 754 to match sprintf behavior, and add half: optional keyword argument for the old behavior. [Bug #12548]

Revision 56590
Added by nobu (Nobuyoshi Nakada) 12 months ago

numeric.c: round to nearest even

  • numeric.c (flo_round, int_round): support round-to-nearest-even semantics of IEEE 754 to match sprintf behavior, and add half: optional keyword argument for the old behavior. [Bug #12548]

Revision 57130
Added by nobu (Nobuyoshi Nakada) 10 months ago

numeric.c: allow nil as rounding mode option

  • numeric.c (rb_num_get_rounding_option): allow nil same as the default behavior, per . [Bug #12548]

Revision 57130
Added by nobu (Nobuyoshi Nakada) 10 months ago

numeric.c: allow nil as rounding mode option

  • numeric.c (rb_num_get_rounding_option): allow nil same as the default behavior, per . [Bug #12548]

Revision 57131
Added by nobu (Nobuyoshi Nakada) 10 months ago

numeric.c: rdoc of half option [ci skip]

  • numeric.c (flo_round): [DOC] mention half option. [Bug #12548]

Revision 57131
Added by nobu (Nobuyoshi Nakada) 10 months ago

numeric.c: rdoc of half option [ci skip]

  • numeric.c (flo_round): [DOC] mention half option. [Bug #12548]

History

#1 [ruby-core:76260] Updated by shyouhei (Shyouhei Urabe) over 1 year ago

I doubt if this is a bug or not. The rounding mode you describe is so-called "round to the nearest even" which is described in ISO 31-0 as "generally preferable". Maybe the author of sprintf explicitly chose such mode (not sure).

It seems the problem here is whether should we specify sprintf's rounding mode or leave it implementation-defined.

#2 [ruby-core:76267] Updated by unclekiki (Kieran McCusker) over 1 year ago

Having read a bit more I see that there are indeed multiple possible ways of handling tie-breaking i.e. when a value is n.5, but is it reasonable for two implementations in ruby core (sprintf and round) to use a different method? It's at least surprising (very in my case) and will catch out anyone coming from other programming backgrounds.

#3 [ruby-core:76273] Updated by shyouhei (Shyouhei Urabe) over 1 year ago

  • Subject changed from Ruby sprintf bug to Rounding modes inconsistency between round versus sprintf

I agree that can be a point. I changed the subject.

#4 [ruby-core:76785] Updated by matz (Yukihiro Matsumoto) about 1 year ago

It should be consistent. Will be fixed.

Matz.

#5 [ruby-core:76936] Updated by noahgibbs (Noah Gibbs) about 1 year ago

It looks like floats are rounded via numeric.c, using the flo_round function. That definitely does not use round-even. Maybe it should? It already implements a rounding function, so it shouldn't be too hard to implement round-even.

It looks like sprintf uses ruby_sprintf, through a series of calls to BSD_vfprintf in vsnprintf.c. At least on my machine. And BSD_vprintf winds up calling ruby_dtoa (#defined from BSD__dtoa) with mode 3. If you look for "round-even" in util.c, you'll see an extensive comment explaining why they used round-even mode.

So basically: right now, Numeric#round uses flo_round(), while sprintf uses util.c's round-even mode.

Presumably we should change flo_round to do round-even.

#6 [ruby-core:76942] Updated by nobu (Nobuyoshi Nakada) about 1 year ago

I agree that round-even is preferable, but it introduces an incompatibility.
Some libraries, e.g., rexml, depend on the current behavior.
So I'm thinking an optional parameter to select rounding mode.

2.5.round        #=> 3
2.5.round(:even) #=> 2

#7 [ruby-core:76953] Updated by noahgibbs (Noah Gibbs) about 1 year ago

Nobu: but then, default will still be inconsistent.

Maybe add a parameter and require libraries to supply it if they care? Then later we could change the default from :even to :closest or :biased or whatever we call the common rounding behavior.

#8 [ruby-core:76956] Updated by shyouhei (Shyouhei Urabe) about 1 year ago

I doubt if we need multiple rounding modes. I guess for every situation when a specific rounding mode is mandatory, what is needed is the round-even mode. I have never experienced the needs of other modes. Almost everybody don't care what mode they use.

Defaulting to that rounding mode should just suffice. Adding parameter solves a problem that doesn't exist.

#9 [ruby-core:76972] Updated by noahgibbs (Noah Gibbs) about 1 year ago

Sounds good to me. Do we need to fix ReXML, if it depends on the current rounding mode?

#10 [ruby-core:76997] Updated by noahgibbs (Noah Gibbs) about 1 year ago

Another reason not to add an argument: Float#round already takes an argument for the number of digits to round to.

#11 [ruby-core:77000] Updated by nobu (Nobuyoshi Nakada) about 1 year ago

Noah Gibbs wrote:

Another reason not to add an argument: Float#round already takes an argument for the number of digits to round to.

It's easy to distinguish a Fixnum and a Symbol.
Or keyword arguments will work well.

#12 [ruby-core:77001] Updated by nobu (Nobuyoshi Nakada) about 1 year ago

Noah Gibbs wrote:

Sounds good to me. Do we need to fix ReXML, if it depends on the current rounding mode?

The rdoc of REXML::Functions.number states:

a string that consists of optional whitespace followed by an optional minus
sign followed by a Number followed by whitespace is converted to the IEEE 754
number that is nearest (according to the IEEE 754 round-to-nearest rule) to
the mathematical value represented by the string; any other string is
converted to NaN

#13 [ruby-core:77004] Updated by noahgibbs (Noah Gibbs) about 1 year ago

Okay, so sounds like just a documentation fix for ReXML. That makes sense. Thanks!

#14 [ruby-core:77265] Updated by noahgibbs (Noah Gibbs) about 1 year ago

Ah - looks like the ReXML docs don't need to change. The round-even mode Ruby uses is still referred to as "round to nearest, ties to even" - https://en.wikipedia.org/wiki/IEEE_floating_point#Roundings_to_nearest

#15 [ruby-core:77339] Updated by noahgibbs (Noah Gibbs) about 1 year ago

  • ruby -v changed from 2.3.1 to 2.4.0
  • File round_even.patch added

Here's a patch that makes floating-point rounding behavior match sprintf()'s round-to-even. It also adds a unit test for the new behavior. The change doesn't break any existing unit tests.

#16 [ruby-core:77344] Updated by nobu (Nobuyoshi Nakada) about 1 year ago

Your patch breaks two tests.

[24/42] TestVector#test_round = 0.00 s             
  1) Failure:
TestVector#test_round [test/matrix/test_vector.rb:162]:
<Vector[1.23, 2.34, 3.4]> expected but was
<Vector[1.23, 2.35, 3.4]>.

[30/42] REXMLTests::FunctionsTester#test_floor_ceiling_round = 0.02 s
  2) Failure:
REXMLTests::FunctionsTester#test_floor_ceiling_round [test/rexml/test_functions.rb:168]:
<[<b id='1'/>]> expected but was
<[]>.

#17 Updated by noahgibbs (Noah Gibbs) about 1 year ago

  • File round_even.patch added

You're right. I apologize. I had been running test-ruby instead of test-all without realizing it.

It appears both tests won't work if we correctly implement the round-even behavior, so the new patch fixes those.

New patch is attached.

#18 Updated by noahgibbs (Noah Gibbs) about 1 year ago

  • File deleted (round_even.patch)

#20 Updated by noahgibbs (Noah Gibbs) about 1 year ago

  • File deleted (round_even.patch)

#21 [ruby-core:77349] Updated by nobu (Nobuyoshi Nakada) about 1 year ago

Noah Gibbs wrote:

It appears both tests won't work if we correctly implement the round-even behavior, so the new patch fixes those.

That is what I had mentioned.

#22 [ruby-core:77350] Updated by nobu (Nobuyoshi Nakada) about 1 year ago

I have a patch too, but it breaks compatibilities in rubyspec.

1)
Float#round returns rounded values for big values FAILED
Expected 200000000000000000000
to have same value and type as 300000000000000000000

spec/rubyspec/core/float/round_spec.rb:70:in `block (2 levels) in <top (required)>'
spec/rubyspec/core/float/round_spec.rb:3:in `<top (required)>'

2)
Integer#round returns itself rounded if passed a negative value FAILED
Expected 200
 to have same value and type as 300

spec/rubyspec/core/integer/round_spec.rb:22:in `block (2 levels) in <top (required)>'
spec/rubyspec/core/integer/round_spec.rb:4:in `<top (required)>'

3)
Rational#round with no arguments (precision = 0) returns the truncated value toward the nearest integer FAILED
Expected 0
 to equal 1

spec/rubyspec/shared/rational/round.rb:17:in `block (3 levels) in <top (required)>'
spec/rubyspec/core/rational/round_spec.rb:3:in `<top (required)>'

4)
Time#strftime rounds an offset to the nearest second when formatting with %z FAILED
Expected "+01:01:04"
 to equal "+01:01:05"

spec/rubyspec/core/time/strftime_spec.rb:50:in `block (2 levels) in <top (required)>'
spec/rubyspec/core/time/strftime_spec.rb:7:in `<top (required)>'

#23 [ruby-core:77356] Updated by noahgibbs (Noah Gibbs) about 1 year ago

nobu (Nobuyoshi Nakada): makes sense. I'll file an issue with RubySpec to let them know that at least some of these tests aren't compatible with IEEE 754 round-nearest-to-even mode. Examining the first test failure (spec/core/float/round_spec.rb line 70), it clearly isn't since it expects 2.5 to round to 3 at that decimal place.

I'll examine the other failures and try to make sure they're all equally clear.

#24 [ruby-core:77358] Updated by noahgibbs (Noah Gibbs) about 1 year ago

Also: looks like your patch fixes it for integer rounding too, based on your RubySpec failures. Mine does not. Sprintf doesn't have an integer rounding mode, but your way is probably still better.

#26 [ruby-core:77690] Updated by noahgibbs (Noah Gibbs) about 1 year ago

Benoit Daloze says that RubySpec should test previous rounding behavior for Ruby before 2.4.0, and test round-even for 2.4.0+. That makes sense to me. Opened RubySpec PR #322 (https://github.com/ruby/spec/pull/322) to reflect this.

When/if we merge Nobu's branch for this bug, we should also ask RubySpec to merge PR #322 or something very similar.

#27 [ruby-core:77706] Updated by noahgibbs (Noah Gibbs) about 1 year ago

Benoit says RubySpec PR #322 looks good. Just waiting for us to commit round-to-nearest-even mode to MRI so he can merge it. I've used Nobu's branch locally, including to create PR #322, and it works well for me. +1

#28 [ruby-core:77961] Updated by nobu (Nobuyoshi Nakada) 12 months ago

We had a discussion today, and the conclusions:

  • add half: optional keyword argument to round methods,
  • its value must be one of :up, :even, and nil,
  • :up is compatible with the existing behavior,
  • :even is the new behavior compatible with sprintf, and
  • if half: option isn't given or is nil, same as :even.

#29 Updated by nobu (Nobuyoshi Nakada) 12 months ago

  • Status changed from Open to Closed

Applied in changeset r56590.


numeric.c: round to nearest even

  • numeric.c (flo_round, int_round): support round-to-nearest-even semantics of IEEE 754 to match sprintf behavior, and add half: optional keyword argument for the old behavior. [Bug #12548]

#30 Updated by Eregon (Benoit Daloze) 11 months ago

  • Related to Bug #12958: Breaking change in how `#round` works added

#31 [ruby-core:78640] Updated by shyouhei (Shyouhei Urabe) 10 months ago

  • Status changed from Closed to Open

Reopening.

The fix was reverted due to issue #12958. This issue is now back alive.

#32 Updated by nobu (Nobuyoshi Nakada) 10 months ago

  • Status changed from Open to Closed

Applied in changeset r57130.


numeric.c: allow nil as rounding mode option

  • numeric.c (rb_num_get_rounding_option): allow nil same as the default behavior, per . [Bug #12548]

#33 [ruby-core:78745] Updated by nobu (Nobuyoshi Nakada) 10 months ago

  • Status changed from Closed to Open
  • Description updated (diff)

Probably, [Feature #10000] can add half option to String#%.

Also available in: Atom PDF