Reduce rounding errors in .decimal_si_to_f, add safe .decimal_si_to_bigdecimal #59

cben · 2018-02-07T13:20:19Z

Currently "87m".decimal_si_to_f == 0.08700000000000001 != 0.087, due to rounding error in multiplication. Many values do result in a "clean" float¹, but around 10%-20% are dirty like this. This PR tries to fix that, and also sidesteps the problem by adding an inherently safe .decimal_si_to_bigdecimal.

¹ Remember that decimal fractions like 0.087 don't have a precise representation in binary floating point; however there is a close float that 0.087 parses to, that also prints back as exactly 0.087, and that's what I'm after — at least being able to print back inputs as we got them.

Changed all fractions in test to "dirty" ones, failing on current implementation.
Changed implementation to run Float("87e-3") instead of Float(87) * 1e-3.
Seems to behave well. Will break with too many decimal digits, around 18.
Added inherently safe .decimal_si_to_bigdecimal.
- Returned decimal precicion will depend on input, but is guaranteed to preserve input digits.

https://bugzilla.redhat.com/show_bug.cgi?id=1504560
@miq-bot add-label bug, gaprindashvili/yes

The only use of this method in ManageIQ is for parsing fractional cpu in millicores.
My main motivation is quota history, see ManageIQ/manageiq-providers-kubernetes#208. While errors here don't affect quotas much after ManageIQ/manageiq-schema#151, because the DB would round small errors from parsing to closest millis, and the _to_f fix here should be enough, I prefer to switch to .decimal_si_to_bigdecimal for quotas.

@zeari Please review.

miq-bot · 2018-02-07T13:20:29Z

@cben Cannot apply the following label because they are not recognized: gaprindashvili/yes

cben · 2018-02-11T12:27:07Z

@enoodle @yaacov Please review

yaacov

nice 👍

p.s.
don't you want to wait with the decimal_si_to_bigdecimal until you want to use it ? (not a blocker)

cben · 2018-02-11T12:40:36Z

I'm going to use it right away, for container quota history.

enoodle

LGTM
Nice test, is there anything special with 93?

cben · 2018-02-11T16:02:56Z

93 is not very special. Some of these had differences with single digits like .3, but some didn't, so chose double digits everywhere for consistent look. The magnitude of the difference tends to bigger for higher fractions. As you can see, I spent too much time on this :-)

zeari · 2018-02-12T08:54:45Z

👍 LGTM

cben · 2018-02-13T11:53:37Z

@bdunne @Fryguy please review.

bdunne · 2018-02-13T13:45:02Z

lib/more_core_extensions/core_ext/string/decimal_suffix.rb

+
+    def decimal_si_to_bigdecimal
+      multiplier = DECIMAL_SUFFIXES[self[-1]]
+      if multiplier


Can this conditional be extracted to a new private method and shared between the two methods?

bdunne · 2018-02-13T13:46:45Z

spec/core_ext/string/decimal_suffix_spec.rb

+    expect("93f".decimal_si_to_f).to eq(0.000_000_000_000_093)
+    expect("92p".decimal_si_to_f).to eq(0.000_000_000_092)
+    expect("93n".decimal_si_to_f).to eq(0.000_000_093)
+    expect("91μ".decimal_si_to_f).to eq(0.000_091)


Is there a reason you're not using the same number for all of the tests?

I wanted test cases that all failed with previous algorithm. This happens for different values semi-randomly, depending on how powers of 2 and 10 align...

[7] pry(main)> 93 * 1e-12 => 9.3e-11 [8] pry(main)> 92 * 1e-12 => 9.199999999999999e-11 [9] pry(main)> 92 * 1e-9 => 9.2e-08 [10] pry(main)> 93 * 1e-9 => 9.300000000000001e-08

Should we change these tests back to the original format and add a new test verifying several values that used to fail for rounding errors or other issues? I see them as two separate issues and wouldn't want to lose this test because someone modified these expectations to consistently use the same number.

I don't see a point having both old and new test, I view 1 as just a particularly easy special case (even changing 1 to 3 makes a couple fail).
I'm not sure if this is what you had in mind, but rewrote to test all input forms over all digit pairs. Total 192/700 failures on the fractional suffixes, all passing after 2nd commit.
PTAL.

bdunne · 2018-02-13T13:47:44Z

spec/core_ext/string/decimal_suffix_spec.rb

+    expect("93f".decimal_si_to_bigdecimal).to eq(BigDecimal("0.000_000_000_000_093"))
+    expect("92p".decimal_si_to_bigdecimal).to eq(BigDecimal("0.000_000_000_092"))
+    expect("93n".decimal_si_to_bigdecimal).to eq(BigDecimal("0.000_000_093"))
+    expect("91μ".decimal_si_to_bigdecimal).to eq(BigDecimal("0.000_091"))


For each fractional suffix, some i, j combinations fail current parsing via integer * factor, e.g. 87 * 0.001 == 0.08700000000000001 != 0.087. (total 192 failures from 700 fractions)

cben · 2018-02-19T16:00:33Z

@bdunne @Fryguy PTAL.
This is needed for ManageIQ/manageiq-providers-kubernetes#198 & ManageIQ/manageiq#16722.

This is a normal gem, needs a release and then bump dependency in manageiq, right?
How does this work for gaprindashvili backport? I see gaprindashvili depends on latest version (3.5) so I suppose I can just bump it equally.

Fryguy · 2018-02-26T17:55:23Z

lib/more_core_extensions/core_ext/string/decimal_suffix.rb

      multiplier = DECIMAL_SUFFIXES[self[-1]]
      if multiplier
-        Float(self[0..-2]) * multiplier
+        self[0..-2] + multiplier


Assuming this is string concat, prefer "#{self[0..-2]}#{multiplier}"

Alternately self[0..-2] << multiplier

Fryguy · 2018-02-26T17:57:15Z

Changed all fractions in test to "dirty" ones, failing on current implementation.

The only thing concerning me with this is whether this is consistent across different people's machines or, more importantly, between versions of Ruby (i.e. as we upgrade). However, I'm not sure how we would handle that kind of testing anyway.

Fryguy · 2018-02-26T17:57:57Z

This is a normal gem, needs a release and then bump dependency in manageiq, right?
How does this work for gaprindashvili backport? I see gaprindashvili depends on latest version (3.5) so I suppose I can just bump it equally.

Yes, we just bump and release this normally, and then in ManageIQ you would just update the dependency.

miq-bot · 2018-02-26T19:45:20Z

Checked commits cben/more_core_extensions@f173bcf~...bd9acf7 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 2 offenses detected

spec/core_ext/string/decimal_suffix_spec.rb

❗ - Line 11, Col 1 - Layout/EmptyLinesAroundArguments - Empty line detected around arguments.
❗ - Line 20, Col 1 - Layout/EmptyLinesAroundArguments - Empty line detected around arguments.

cben · 2018-02-26T21:58:13Z

The 7 "dirty" fractions fail the old implementation and pass the new one on:

Intel i7-5600U, ruby 2.0.0-p648
Intel i7-5600U, ruby 2.3.4
Intel i7-5600U, ruby 2.4.1
Intel i7-5600U, ruby 2.5.0-dev.
ARMv7 BCM2835 (raspberry pi 3 😁), ruby 2.1.5p273 [arm-linux-gnueabihf].

CORRECTION: there are no longer 7 specific fractions, I'm testing 100 values at each scale. But the first failure at each scale is same between Intel and ARM — I guess IEEE did a good job...

Added - String#decimal_si_to_big_decimal [[#59](#59)]

bdunne · 2018-03-01T15:50:48Z

@cben v3.6.0 has been released with your changes.

cben · 2018-03-04T09:30:36Z

Thanks!

ManageIQ/more_core_extensions#59 ManageIQ/more_core_extensions#60

miq-bot added the bug label Feb 7, 2018

yaacov approved these changes Feb 11, 2018

View reviewed changes

enoodle approved these changes Feb 11, 2018

View reviewed changes

zeari approved these changes Feb 12, 2018

View reviewed changes

bdunne requested changes Feb 13, 2018

View reviewed changes

cben added 3 commits February 14, 2018 12:41

decimal_si_to_f test: check values that suffer from rounding errors

f173bcf

For each fractional suffix, some i, j combinations fail current parsing via integer * factor, e.g. 87 * 0.001 == 0.08700000000000001 != 0.087. (total 192 failures from 700 fractions)

decimal_si_to_f: avoid rounding error in parsing by letting Float do it

a873f0a

Add String#decimal_si_to_bigdecimal method

7abecb9

cben force-pushed the decimal_si_precise branch from caf14fe to 7abecb9 Compare February 14, 2018 10:41

This was referenced Feb 19, 2018

Keep quota history by archiving ManageIQ/manageiq-providers-kubernetes#198

Merged

Keep container quota history by archiving ManageIQ/manageiq#16722

Merged

Fryguy reviewed Feb 26, 2018

View reviewed changes

review feedback

bd9acf7

bdunne approved these changes Feb 28, 2018

View reviewed changes

bdunne merged commit 02695be into ManageIQ:master Feb 28, 2018

bdunne self-assigned this Feb 28, 2018

bdunne mentioned this pull request Feb 28, 2018

Rename and add documentation for String#decimal_si_to_big_decimal #60

Merged

bdunne added a commit that referenced this pull request Mar 1, 2018

Release v3.5.0

8aef00f

Added - String#decimal_si_to_big_decimal [[#59](#59)]

cben added a commit to cben/manageiq-providers-kubernetes that referenced this pull request Mar 5, 2018

Bump more_core_extensions to 3.6.0, including decimal_si_to_big_decimal

9805f9d

ManageIQ/more_core_extensions#59 ManageIQ/more_core_extensions#60

cben added a commit to cben/manageiq-providers-kubernetes that referenced this pull request Mar 5, 2018

Bump more_core_extensions to 3.6.0, including decimal_si_to_big_decimal

67c43fc

ManageIQ/more_core_extensions#59 ManageIQ/more_core_extensions#60

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce rounding errors in .decimal_si_to_f, add safe .decimal_si_to_bigdecimal #59

Reduce rounding errors in .decimal_si_to_f, add safe .decimal_si_to_bigdecimal #59

cben commented Feb 7, 2018

miq-bot commented Feb 7, 2018

cben commented Feb 11, 2018

yaacov left a comment

cben commented Feb 11, 2018 via email

enoodle left a comment

cben commented Feb 11, 2018 via email •

edited

Loading

zeari commented Feb 12, 2018

cben commented Feb 13, 2018

bdunne Feb 13, 2018

cben Feb 13, 2018

bdunne Feb 13, 2018

cben Feb 13, 2018

bdunne Feb 13, 2018

cben Feb 14, 2018

bdunne Feb 13, 2018

cben commented Feb 19, 2018

Fryguy Feb 26, 2018

Fryguy Feb 26, 2018

cben Feb 26, 2018

Fryguy commented Feb 26, 2018

Fryguy commented Feb 26, 2018

miq-bot commented Feb 26, 2018

cben commented Feb 26, 2018 •

edited

Loading

bdunne commented Mar 1, 2018

cben commented Mar 4, 2018

Reduce rounding errors in .decimal_si_to_f, add safe .decimal_si_to_bigdecimal #59

Reduce rounding errors in .decimal_si_to_f, add safe .decimal_si_to_bigdecimal #59

Conversation

cben commented Feb 7, 2018

miq-bot commented Feb 7, 2018

cben commented Feb 11, 2018

yaacov left a comment

Choose a reason for hiding this comment

cben commented Feb 11, 2018 via email

enoodle left a comment

Choose a reason for hiding this comment

cben commented Feb 11, 2018 via email • edited Loading

zeari commented Feb 12, 2018

cben commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cben commented Feb 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Feb 26, 2018

Fryguy commented Feb 26, 2018

miq-bot commented Feb 26, 2018

cben commented Feb 26, 2018 • edited Loading

bdunne commented Mar 1, 2018

cben commented Mar 4, 2018

cben commented Feb 11, 2018 via email •

edited

Loading

cben commented Feb 26, 2018 •

edited

Loading