search

ManageIQ / more_core_extensions

MoreCoreExtensions are a set of core extensions beyond those provided by ActiveSupport.
MIT License
5 stars 23 forks source link

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

Closed cben closed 6 years ago

cben commented 6 years ago

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.

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 https://github.com/ManageIQ/manageiq-providers-kubernetes/issues/208. While errors here don't affect quotas much after https://github.com/ManageIQ/manageiq-schema/pull/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 commented 6 years ago

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

cben commented 6 years ago

@enoodle @yaacov Please review

cben commented 6 years ago

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

cben commented 6 years ago

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 commented 6 years ago

:+1: LGTM

cben commented 6 years ago

@bdunne @Fryguy please review.

cben commented 6 years ago

@bdunne @Fryguy PTAL. This is needed for https://github.com/ManageIQ/manageiq-providers-kubernetes/pull/198 & https://github.com/ManageIQ/manageiq/pull/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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Checked commits https://github.com/cben/more_core_extensions/compare/f173bcf47ea9380134c0a20069ef00c37b7e27ba~...bd9acf739f503e6dbfad40dd538fd59bb3dceed9 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

cben commented 6 years ago

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

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...

bdunne commented 6 years ago

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

cben commented 6 years ago

Thanks!