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

Update the double/single formatter to return the shortest roundtrippable string #11574

Closed
tannergooding opened this issue Nov 29, 2018 · 5 comments
Closed

Comments

@tannergooding
Copy link
Member

@tannergooding tannergooding commented Nov 29, 2018

This was broken out of https://github.com/dotnet/coreclr/issues/19802

Currently we, when using the R specifier, attempt to format to 15 digits, roundtrip back, and if the value does not equal, we reformat to 17 digits and return that value. This has a number of issues, including that it is less performant and is not always correct. The simplest fix is to always roundtrip to 17 digits, but this has the undesired side-effect of returning less user-friendly strings for some numbers.

A more ideal fix would be to instead return the shortest roundtrippable string for a given value. There are several whitepapers on the subject and our current formatting algorithms (Dragon4 and Grisu3) both have sections in their whitepapers that cover this. This provides a good balance between being "correct" and returning "user-friendly" strings.

@tannergooding
Copy link
Member Author

@tannergooding tannergooding commented Nov 29, 2018

CC. @mazong1123 who worked on the original improvements and added the Dragon4/Grisu3 implementations

@tannergooding
Copy link
Member Author

@tannergooding tannergooding commented Nov 29, 2018

@juliusfriedman
Copy link
Contributor

@juliusfriedman juliusfriedman commented Nov 29, 2018

My only real problem and hence my downvote would be that given the consideration that all things are not equal; Why not have a slightly different design which allows for the algorithm to be easily switched out?

Certain systems are addressed in multiple ways not just linearly but also scalar and reserved of the two[or whatever other variation therein] utilitied.

+-+=

With that being typed I will also say that this should be given a good consideration and probably approved however another issue should be opened to enhance the functionality with lower priority. Imho.

@tannergooding
Copy link
Member Author

@tannergooding tannergooding commented Nov 29, 2018

@juliusfriedman I'm not sure why this wouldn't work for both.

We have "R" which is meant to return a round-trippable string. It is currently broken/buggy so we want to fix it while still providing user-friendly strings. That is, if the user specifies "R", we shouldn't return 1.1000000000000001 when 1.1 is perfectly suitable.

This will not mean that we return the shortest roundtrippable string in all cases. If the user explicitly gives us a precision specifier (such as G17 or G99) we would return the number of digits requested and in the format they requested.

It may also be worth considering that R today, eats and ignores the precision specifier. It may be possible to start listening to the precision specifier and:

  • When the requested precision is less than the shortest roundtrippable string, we return the shortest roundtrippable string
  • When the requested precision is greater than the shortest roundtrippable string, we print the additional digits requested (since that will also roundtrip correctly)

@tannergooding
Copy link
Member Author

@tannergooding tannergooding commented Feb 1, 2019

Now that dotnet/coreclr#22040 is merged, for .NET Core 3.0, ToString(), ToString("G"), ToString("R"), double.ToString("G17"), and float.ToString("G9") should now always produce a string that will roundtrip to the original value..

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
@msftbot msftbot bot locked as resolved and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants