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

round trip issue #4

Open
matunia opened this issue Nov 20, 2019 · 5 comments
Open

round trip issue #4

matunia opened this issue Nov 20, 2019 · 5 comments

Comments

@matunia
Copy link

matunia commented Nov 20, 2019

during tests on random values I've found several cases where the generated string won't do the round trip to the original value e.g.:

431.45061948654927 -> 431.45061948654933
27.201309524416448 ->27.201309524416452
...

@rotanov
Copy link

rotanov commented Jan 5, 2023

Indeed those two do not pass.
I also stumbled upon this with 0.51211952535068594.

I fixed running nunit tests with vs2022 here https://github.com/rotanov/grisu.net/tree/fix_nunit
and added your (@matunia) test cases plus mine here https://github.com/rotanov/grisu.net/tree/nonpassing_test_cases

image

Also last time I noted this it seemed that it's was only possible to reproduce on:

  • Ryzen 7 3700X
  • Ryzen 9 3900X

But it didn't fail on Ryzen 7 2700X.

And now it fails again on i7-11800H.

Also at one point I thought maybe if something wrong with Double.Parse, maybe something CPU dependent. Now I don't remember why I thought so and no longer sure if issue is CPU dependent or not. Just in case I tried compiling everything with .NET7 and that didn't fix the issue.

Judging by pull requests may be @shunter can help somehow?

@kring
Copy link
Owner

kring commented Jan 9, 2023

It's probably worth checking the implementation against the original C++ implementation, especially if there were changes since it was ported. I'm afraid I haven't used this project for several years now, so I don't have any particular insight into what might be wrong.

@rotanov
Copy link

rotanov commented Jan 9, 2023

Alright, this seems to be the valid mirror for the original C++ implementation (https://github.com/stlab/double-conversion). I'll return with a pull request if I manage to locate the issue when I have time for that. Thanks!

@rotanov
Copy link

rotanov commented Jan 9, 2023

I compiled original C++ grisu, run my test case and it's exhibits the same behavior as C# port:

image

But! strtod parses 0.512119525350686 back to 0.51211952535068594 and the first one is indeed shorter. And Double.Parse in C# does not:

image

@rotanov
Copy link

rotanov commented Jan 9, 2023

Maybe the behavior in Double.Parse is related to this issue: dotnet/runtime#4406.

edit: I 95% sure it's the source of the issue. Resolution is to use IEEE compliant double parser instead of C#'s Double.Parse.

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

No branches or pull requests

3 participants