Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

units.parse has inconsistent rounding errors for values of 0.5 #4856

Closed
tmos22 opened this issue Jul 7, 2022 · 1 comment · Fixed by #4857
Closed

units.parse has inconsistent rounding errors for values of 0.5 #4856

tmos22 opened this issue Jul 7, 2022 · 1 comment · Fixed by #4857
Assignees
Labels

Comments

@tmos22
Copy link

tmos22 commented Jul 7, 2022

Short description

units.parse has rounding errors when calculating 0.5 during conversion with the milli- suffix (lowercase m) and Kilo- (k/K). All other prefixes seem to calculate properly.

System Details:

  • OPA version: >= v0.41.0 (checked against both 0.41.0 and 0.42.0)
  • OS: RHEL 7.9, UEK 5.4.17
  • Go Version: go1.16.4 linux/amd64
  • Built locally with the following command/flags
 go build -o opa -ldflags "\
		-X github.com/open-policy-agent/opa/internal/report.ExternalServiceURL='' \
		-X github.com/open-policy-agent/opa/version.Version=local-unofficial \
		-X github.com/open-policy-agent/opa/version.Vcs=`./build/get-build-commit.sh` \
		-X github.com/open-policy-agent/opa/version.Timestamp=`./build/get-build-timestamp.sh` \
		-X github.com/open-policy-agent/opa/version.Hostname=`./build/get-build-hostname.sh`"

Steps To Reproduce

  1. Execute opa run
  2. Run units.parse("500m")
  3. Receive rounding error of 0.5000000000000000104
  4. Run units.parse("0.0005K")
  5. Receive rounding error of 0.49999999999999999997
> units.parse("0.0000000000000000005E")
0.5
> units.parse("0.0000000000000005P")
0.5
> units.parse("0.0000000000005T")
0.5
> units.parse("0.0000000005G")
0.5
> units.parse("0.0000005M")
0.5
> units.parse("0.0005K")
0.49999999999999999997
> units.parse("0.5")
0.5
> units.parse("500m")
0.5000000000000000104

# Testing equality
> units.parse("0.0000000000000000005E") == 0.5
true
> units.parse("0.0000000000000005P") == 0.5
true
> units.parse("0.0000000000005T") == 0.5
true
> units.parse("0.0000000005G") == 0.5
true
> units.parse("0.0000005M") == 0.5
true
> units.parse("0.0005K") == 0.5
false
> units.parse("0.5") == 0.5
true
> units.parse("500m") == 0.5
false

Expected behavior

All units that should be parsed to the same value should equal the same value (all 0.5s should be equal)

> units.parse("500m")
0.5
> units.parse("0.0005K")
0.5
@tmos22 tmos22 added the bug label Jul 7, 2022
@philipaconrad philipaconrad self-assigned this Jul 7, 2022
@philipaconrad
Copy link
Contributor

@tmos22 Thanks for reporting this! I was able to reproduce the issue locally from your test cases, and I have a pretty good idea of how to fix the issue. Internally, it's caused by floating-point rounding issues, even though OPA uses big.Float internally for the units parsing builtin.

The solution is probably going to involve normalizing the path that incoming units quantities go through, so that they experience identical rounding behavior, regardless of whether we're dealing with milli or Kilo units.

philipaconrad added a commit to philipaconrad/opa that referenced this issue Jul 7, 2022
Previously, we used big.Float for units parsing, but this resulted in
occasional rounding issues, due to values like 0.001 not being perfectly
representable in floating-point format.

This issue was fixed by switching units parsing to use big.Rat, since
Rationals can generally handle such quantities with perfect precision.

Fixes open-policy-agent#4856.

Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
philipaconrad added a commit that referenced this issue Jul 7, 2022
Previously, we used big.Float for units parsing, but this resulted in
occasional rounding issues, due to values like 0.001 not being perfectly
representable in floating-point format.

This issue was fixed by switching units parsing to use big.Rat, since
Rationals can generally handle such quantities with perfect precision.

Fixes #4856.

Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants