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

Regression in float stringification - dragonbox [devel] #18182

Closed
kaushalmodi opened this issue Jun 4, 2021 · 8 comments
Closed

Regression in float stringification - dragonbox [devel] #18182

kaushalmodi opened this issue Jun 4, 2021 · 8 comments

Comments

@kaushalmodi
Copy link
Contributor

kaushalmodi commented Jun 4, 2021

This is a recent regression in float stringification on devel.

Example

import std/[unittest]
import parsetoml # nimble install parsetoml

test "TOML float":
  check $("some_float = 0.123".parseString()["some_float"].getFloat()) == "0.123"

Current Ouput on devel

/home/kmodi/sandbox/nim/parsetoml/tests/t0p123.nim(5, 71): Check failed: $("some_float = 0.123".parseString()["some_float"].getFloat()) == "0.123"
$("some_float = 0.123".parseString()["some_float"].getFloat()) was 0.12300000000000001
[FAILED] TOML float

Expected output

[OK] TOML float

The test is passing on the latest stable on Nim Playground: https://play.nim-lang.org/#ix=3oME

Breaks parsetoml test

This issue was found as the Travis CI weekly cron for parsetoml failed today: https://travis-ci.org/github/NimParsers/parsetoml/builds/773550319

Values for key 'zero-intpart' don't match. Expected a value of '0.123' but got '0.12300000000000001'.
131 passed, 1 failed

To run that parsetoml test suite, do:

git clone https://github.com/NimParsers/parsetoml
nimble run_toml_test

Above runs a standard TOML spec test suite from https://github.com/BurntSushi/toml-test .

This particular test from that suite is failing:

Nim version

> nim -v
Nim Compiler Version 1.5.1 [Linux: amd64]
Compiled at 2021-06-04
Copyright (c) 2006-2021 by Andreas Rumpf

git hash: 5423915e5ade14fb98c2aba28f269311506960e5
active boot switches: -d:release

/cc @PMunch @timotheecour @Araq

@timotheecour
Copy link
Member

unclear whether this is a bug until a minimized test case is provided.
note that the following is correct:

nim --eval:'var a = 0.1; echo a+0.2'
0.30000000000000004

whereas in 1.4.4 you'd have 0.3, which is incorrect:
echo 'var a = 0.1; echo a+0.2' | nim r -

@kaushalmodi
Copy link
Contributor Author

@timotheecour I have updated my main post with a minimal example and more details.

@timotheecour
Copy link
Member

timotheecour commented Jun 5, 2021

IMO this is a parsetoml bug (and should be fixed!), not a stdlib bug:

import parsetoml # nimble install parsetoml
let a2 = "some_float = 0.123".parseString()["some_float"]
let a4 = a2.getFloat()
echo (a4, a4 == 0.123)

in 1.4.4: prints (0.123, false)
in devel a2b6081: prints (0.12300000000000001, false)

note that stdlib parsing works, eg:

import strutils
doAssert "0.123".parseFloat == 0.123 # works

@kaushalmodi
Copy link
Contributor Author

kaushalmodi commented Jun 5, 2021

@timotheecour

IMO this is a parsetoml bug (and should be fixed!), not a stdlib bug:

I was beginning to think the same as I was trying to create a minimal example without parsetoml.

Below is a gist of what's causing this bug from

https://github.com/NimParsers/parsetoml/blob/376086763a5327478d44a0d61c774d2352668b0c/src/parsetoml.nim#L271-L299

import math, sugar

var
  invPowerOfTen = 10.0
  decimalPart: float64

for ch in ['1', '2', '3']: # 0.123
  decimalPart += (int(ch) - int('0')).float / invPowerOfTen
  invPowerOfTen *= 10

dump decimalPart
doAssert $decimalPart == "0.123"

On devel

Output:

decimalPart = 0.12300000000000001
/home/kmodi/sandbox/nim/bug_reports/t18182.nim(12) t18182
/home/kmodi/usr_local/apps/7/nim/devel/lib/system/assertions.nim(39) failedAssertImpl
/home/kmodi/usr_local/apps/7/nim/devel/lib/system/assertions.nim(29) raiseAssert
/home/kmodi/usr_local/apps/7/nim/devel/lib/system/fatal.nim(53) sysFatal
Error: unhandled exception: /home/kmodi/sandbox/nim/bug_reports/t18182.nim(12, 10) `$decimalPart == "0.123"`

On stable

https://play.nim-lang.org/#ix=3oML

Output:

decimalPart = 0.123

@kaushalmodi
Copy link
Contributor Author

I am closing this issue as I have opened NimParsers/parsetoml#45.

@kaushalmodi
Copy link
Contributor Author

kaushalmodi commented Jun 5, 2021

@timotheecour

OK! Found the super minimal reproduction of the error:

nim --eval:'assert 0.1 + 0.02 == 0.12'

Above fails on devel, which I believe is a limitation of the float type.. But the fact that this passed in stable doesn't make this a serious regression?

Update: It looks like earlier Nim implicitly did something like what the Decimal module does in Python: https://docs.python.org/3/library/decimal.html . But now it's not.

So while Nim did fix the float point math, what should users use to get the Decimal-like outcome where 0.1 + 0.02 == 0.12?

@kaushalmodi kaushalmodi reopened this Jun 5, 2021
@timotheecour
Copy link
Member

OK! Found the super minimal reproduction of the error:

still works as intended in devel.
with 1.4.4:
echo 0.1 + 0.02 prints 0.12, but echo 0.1 + 0.02 == 0.12 prints false, because of how FP works.

with devel, echo 0.1 + 0.02 prints 0.12000000000000001, which is now consistent.

But the fact that this passed in stable doesn't make this a serious regression?

it doesn't, echo 'echo 0.1 + 0.02 == 0.12' | $nim_144_X r - prints false

Update: It looks like earlier Nim implicitly did something like what the Decimal module does in Python: docs.python.org/3/library/decimal.html . But now it's not.

nothing to do with python's decimals semantics, it just showed 0.12 "by accident"; because it used sprintf which is incorrect wrt roundtrip

So while Nim did fix the float point math, what should users use to get the Decimal-like outcome where 0.1 + 0.02 == 0.12?

don't expect 0.1 + 0.02 == 0.12 with FP math, instead use a decimal library for that, eg 0.1'dec + 0.02'dec == 0.12'dec, refs #17699

@kaushalmodi
Copy link
Contributor Author

Thanks, I'm closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants