fix: prettier floating point numbers by SGSSGene · Pull Request #1293 · jbeder/yaml-cpp

SGSSGene · 2024-07-06T06:11:02Z

This PR adds a new function fp_to_string.
fp_to_string internally uses dragonbox to compute the required precision to print floating point numbers. This avoids uglification of floating point numbers that happen by default via std::stringstream.

Numbers like 34.34 will be converted to '34.340000000000003' as strings. With this version they will be converted to the string '34.34'.

adjust dragonbox.h so everything is wrapped in a YAML namespace
discuss possible license adjustments for dragonbox.h
move dragonbox.h to contrib
requires OK of licensing from jk-jeon
make sure this solves the issue.

jbeder · 2024-07-07T20:24:38Z

@jk-jeon, given your expertise, would you be willing to review this?

SGSSGene · 2024-07-14T21:41:06Z

@Anton3: Could you check that this PR is actually solving your issue?

@jk-jeon: Can you check if the license in changes in a include/yaml-cpp/contrib/dragonbox.h to your liking? (I have tripled licensed them).

I belive this PR is ready to go.

jk-jeon · 2024-07-15T01:58:07Z

@jk-jeon: Can you check if the license in changes in a include/yaml-cpp/contrib/dragonbox.h to your liking? (I have tripled licensed them).

Looks good. Thanks!

davidzchen · 2024-07-15T20:10:43Z

It looks like the indentation style of the new code is inconsistent with that of the rest of the codebase (4 vs 2 spaces). @SGSSGene can you make the style consistent?

SGSSGene · 2024-07-16T11:43:55Z

It looks like the indentation style of the new code is inconsistent with that of the rest of the codebase (4 vs 2 spaces). @SGSSGene can you make the style consistent?

Yes, very good points.

Should I also make a fp_to_string.cpp file? Most things don't really have to be in the header file.

SGSSGene · 2024-07-30T09:55:22Z

It looks like the indentation style of the new code is inconsistent with that of the rest of the codebase (4 vs 2 spaces). @SGSSGene can you make the style consistent?

Yes, very good points.

Should I also make a fp_to_string.cpp file? Most things don't really have to be in the header file.

I split fp_to_string.h into src/fptostring.cpp and include/yaml-cpp/fptostring.h. This also allowed to move dragonbox.h into src/contrib folder.

SGSSGene · 2024-07-30T10:09:13Z

@jbeder an you help me out with the current windows-shared-build errors? (I currently don't have a windows machine at hand). I thought adding YAML_CPP_API in fptostring.h would be enough. But it seems like the symbols are not being exported into the DLL?

jbeder · 2024-07-30T14:13:43Z

Honestly I don't know, I don't have a Windows machine any more either :)

Maybe ask on Stack Overflow? I'd be shooting in the dark also.

SGSSGene · 2024-07-30T18:46:32Z

Honestly I don't know, I don't have a Windows machine any more either :)

Maybe ask on Stack Overflow? I'd be shooting in the dark also.

Found the issue: the fptostring.cpp didn't include fptostring.h, which than didnt see the "YAML_CPP_API" declaration, which then didn't export the symbols into the dll. Fixed now!

Ready to go from my side :-)

Anton3 · 2024-08-27T12:22:01Z

@jbeder Could you give another round of review, please? Can't wait to stop uglifying my configs :)

SGSSGene · 2024-09-10T08:56:03Z

Hi @jbeder , any chance you can take another look?

SGSSGene · 2024-09-23T10:55:56Z

Hey @jbeder, a polite reminder that this is still open :-)

davidzchen · 2024-09-29T02:03:28Z

Sorry for the delay. Changes LGTM on my side.

@jbeder Can you please take a look at this?

davidzchen · 2024-10-14T06:09:18Z

@jbeder Are there any other blockers before this can be merged?

SGSSGene · 2024-10-21T09:45:15Z

@jbeder, any thing we can do, to motivate you looking at this PR?

jbeder

Sorry, I've been busy. Also, has the owner of dragonbox approved this PR?

jk-jeon · 2024-10-21T19:10:46Z

Also, has the owner of dragonbox approved this PR?

I approved the license notification.
If you are talking about the implementation itself, I still think precision specification is fundamentally incompatible with the shortest roundtripping representation and don't recommend mixing them together, but if 100% correct rounding/roundtrip is not your goal than it seems okay.

Add dragonbox to compute the required precision to print floating point numbers. This avoids uglification of floating point numbers that happen by default via std::stringstream. Numbers like 34.34 used to be converted to '34.340000000000003' as strings. With this version they will be converted to the string '34.34'. This fixes issue jbeder#1289

SGSSGene · 2024-11-07T10:29:42Z

@jbeder Hey jbeder, friendly reminder, I made the adjustments you suggested to make.

SGSSGene mentioned this pull request Jul 6, 2024

Floating-point numbers are uglified, a.k.a. write shortest floating-point representation with round-trip guarantee #1289

Closed

SGSSGene force-pushed the fix/ugly_floats branch 7 times, most recently from 8a3c7b3 to b81ffd7 Compare July 14, 2024 21:35

SGSSGene changed the title ~~[NOMERGE] fix: prettier floating point numbers~~ fix: prettier floating point numbers Jul 14, 2024

davidzchen reviewed Jul 15, 2024

View reviewed changes

Comment thread include/yaml-cpp/fp_to_string.h Outdated

davidzchen reviewed Jul 15, 2024

View reviewed changes

Comment thread include/yaml-cpp/fp_to_string.h Outdated

SGSSGene force-pushed the fix/ugly_floats branch 2 times, most recently from dda0397 to 1fe5866 Compare July 18, 2024 17:44

SGSSGene requested a review from davidzchen July 22, 2024 08:25

SGSSGene force-pushed the fix/ugly_floats branch 2 times, most recently from 4ccb3a6 to acf2e70 Compare July 30, 2024 09:53

SGSSGene force-pushed the fix/ugly_floats branch 2 times, most recently from f40095f to f4368b1 Compare July 30, 2024 18:37

davidzchen approved these changes Sep 29, 2024

View reviewed changes

jbeder requested changes Oct 21, 2024

View reviewed changes

Comment thread include/yaml-cpp/fptostring.h Outdated

Comment thread src/fptostring.cpp Outdated

Comment thread src/fptostring.cpp

SGSSGene added 10 commits October 21, 2024 22:49

fix: rename fp_to_string to FpToString to match coding style

9b27962

fix: fp_to_string uses now 2 spaces

54dd046

fix: remove trailing return types

e63f4da

doc: add a little more information to dragonbox file

58b7a3c

patch: split fp_to_string.h into fptostring.h and fptostring.cpp

6dcc526

patch: move dragonbox.h from 'include' folder to 'src' folder

07edac1

fix: add YAML_CPP_API for windows-shared library builds

209c190

fix: add missing include

4f9e01e

remove reduntant license information

bb792ce

SGSSGene force-pushed the fix/ugly_floats branch from f4368b1 to bb792ce Compare October 21, 2024 20:50

jbeder requested changes Oct 22, 2024

View reviewed changes

Comment thread src/fptostring.cpp

SGSSGene added 2 commits October 22, 2024 19:52

patch: added more defensive programming techniques

e3d3041

fix: spell fix

a5c162f

SGSSGene requested a review from jbeder October 22, 2024 18:53

jbeder approved these changes Nov 7, 2024

View reviewed changes

jbeder merged commit 9ce5a25 into jbeder:master Nov 7, 2024

Conversation

SGSSGene commented Jul 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbeder commented Jul 7, 2024

Uh oh!

SGSSGene commented Jul 14, 2024

Uh oh!

jk-jeon commented Jul 15, 2024

Uh oh!

davidzchen commented Jul 15, 2024

Uh oh!

Uh oh!

Uh oh!

SGSSGene commented Jul 16, 2024

Uh oh!

SGSSGene commented Jul 30, 2024

Uh oh!

SGSSGene commented Jul 30, 2024

Uh oh!

jbeder commented Jul 30, 2024

Uh oh!

SGSSGene commented Jul 30, 2024

Uh oh!

Anton3 commented Aug 27, 2024

Uh oh!

SGSSGene commented Sep 10, 2024

Uh oh!

SGSSGene commented Sep 23, 2024

Uh oh!

davidzchen commented Sep 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidzchen commented Oct 14, 2024

Uh oh!

SGSSGene commented Oct 21, 2024

Uh oh!

jbeder left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jk-jeon commented Oct 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

SGSSGene commented Nov 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

SGSSGene commented Jul 6, 2024 •

edited

Loading

davidzchen commented Sep 29, 2024 •

edited

Loading

jk-jeon commented Oct 21, 2024 •

edited

Loading