Use BigDecimal compatible operation in NumberToRoundedConverter

[federicoaldunate]

Summary

NumberToRoundedConverter uses operations with BigDecimal that will show inaccurate results due to the fact that the number is converted to Float to use that operation. Inaccuracies will arise when Float cannot accurately store the number.
Example

"%00.3f" % 0.287298702e23.to_d
=> "28729870199999998984192.000"

require 'bigdecimal'
require 'bigdecimal/util'

# when convertion is accurate from BigDecimal to Float
BigDecimal('2872987020000001.0') == BigDecimal('2872987020000001.0').to_f
=> true
"%00.1f" % BigDecimal('2872987020000001.0')
=> "2872987020000001.0"
"%00.1f" % BigDecimal('2872987020000001.0') == BigDecimal('2872987020000001.0').to_s('F')
=> true

# when convertion is not accurate from BigDecimal to Float
BigDecimal('28729870200000001.0') == BigDecimal('28729870200000001.0').to_f
=> false
"%00.1f" % BigDecimal('28729870200000001.0')
=> "28729870200000000.0"
"%00.1f" % BigDecimal('28729870200000001.0') == BigDecimal('28729870200000001.0').to_s('F')
=> false

So any BigDecimal number that cannot be expressed as a float accurately will make this operation inaccurately.

This is an attempt to fix #42302

Other Information

Now that rounded_number == rounded_number.to_i condition is not needed I changed the if...else to return 'Nan' when rounded_number.nan? and 'Inf' when rounded_number.infinite? instead of "%00.#{precision}f" % rounded_number that returns the same string for those cases

[zzak]

LGTM @tom-lord wdyt?

[pixeltrix]

It's a bit of a micro-optimisation but we can add a elsif precision.zero? clause there and avoid some object allocations:

  if rounded_number.nan?
    "NaN"
  elsif rounded_number.infinite?
    "Inf"
  elsif precision.zero?
    "%i" % rounded_number
  else
    s = rounded_number.to_s("F")
    s << "0" * precision
    a, b = s.split(".", 2)
    a << "."
    a << b[0, precision]
  end

Could even go one step further and check for frac.zero? like this:

  elsif rounded_number.frac.zero?
    "%i.#{'0' * precision}" % rounded_number

Unsure about the relative performance of frac though on BigDecimal.

[federicoaldunate]

It's a bit of a micro-optimisation but we can add a elsif precision.zero? clause there and avoid some object allocations:

  if rounded_number.nan?
    "NaN"
  elsif rounded_number.infinite?
    "Inf"
  elsif precision.zero?
    "%i" % rounded_number
  else
    s = rounded_number.to_s("F")
    s << "0" * precision
    a, b = s.split(".", 2)
    a << "."
    a << b[0, precision]
  end

Could even go one step further and check for frac.zero? like this:

  elsif rounded_number.frac.zero?
    "%i.#{'0' * precision}" % rounded_number

Unsure about the relative performance of frac though on BigDecimal.

My intuition said you were right, but I did some (simple) benchmark to test it out and it seems that the code with those allocations is faster (I've also added some changes to the code to make it a little bit faster).
https://gist.github.com/federicoaldunate/b4903302576a229ed8f08e6a7828d616

[pixeltrix]

@federicoaldunate nice - I guess the parsing of the format string by % or sprintf is what causes the performance drop. Is it worth moving the a << b[0, precision] inside the conditional and return a as the last line?

[federicoaldunate]

@pixeltrix Did it, benchmark showed a little improvement in performance so I updated the code.

[pixeltrix]

@federicoaldunate thanks for bearing with my suggestions and checking them out 👍

[zzak]

Nice work @federicoaldunate!!!

[tom-lord]

Hey, sorry for being slow replying here, you caught me on a bank holiday weekend with great weather 😉

I like the removal of the rounded_number == rounded_number.to_i check, that definitely felt a little dubious when I wrote it. And handling the digits construction in a single unified way likewise seems cleaner.

However, I also think this PR could have introduced another slight regression: consider -Float::INFINITY:

"%f" % Float::INFINITY
  #=> "Inf"
  "%f" % -Float::NAN
 #=>"NaN"
"%f" % -Float::INFINITY
  #=> "-Inf" (!!!!)

The new implementation of this method will format -Float::INFINITY as Inf instead of -Inf.

Weirdly, also note that Float::INFINITY.to_s == "Infinity", not "Inf". So whilst that could be an acceptable solution(?) it's a change of behaviour.

Or alternatively, rather than hardcoding "-Inf" as well as the other two values, perhaps the following implementation would be the best workaround?...

          formatted_string =
            if rounded_number.finite?
              s = rounded_number.to_s("F")
              a, b = s.split(".", 2)
              if precision != 0
                b << "0" * precision
                a << "."
                a << b[0, precision]
              end
              a
            else
              # Infinity/NaN
              "%f" % rounded_number
            end

[zzak]

@tom-lord Thanks for checking back and hope you had a good holiday :)

I think I like your proposal, would you be willing to work on a PR? If not I can give it a shot 🙌

[zzak]

@tom-lord #42341 should take care of it 🙇

[zzak]

@pixeltrix Since you merged this, do you think it is worth backporting to 6.1?