Make the JSON and XML archives round trip floating point values exactly - #775
Open
DanNegrut wants to merge 1 commit into
Open
Make the JSON and XML archives round trip floating point values exactly#775DanNegrut wants to merge 1 commit into
DanNegrut wants to merge 1 commit into
Conversation
ChArchiveOutJSON and ChArchiveOutXML wrote floating point values at the default ostream precision, which is 6 significant digits, while a double needs 17 to be recovered exactly. Saving a model to JSON or XML and loading it back therefore returned different numbers, silently. Measured, with the binary backend as a control since it writes raw bytes rather than printing them: original JSON XML binary 0.33333333333333331 0.333333 0.333333 exact 12345.678901234567 12345.7 12345.7 exact Note the second row: six significant digits is a relative budget, so the absolute error grows with magnitude, reaching about 0.022 there. Anyone using JSON or XML to checkpoint and restart a simulation was not restarting from where they stopped. The fix raises the precision to max_digits10, which is the number of decimal digits that uniquely distinguishes every value of a type. It is set on the stream in the output archive's constructor and restored in the destructor, because the stream belongs to the caller and may be reused afterwards; std::cout, for one, should not be left permanently reconfigured. There is a test for that restoration. The float overloads set 9 rather than inheriting the double's 17. Printing a float with eight digits more than it carries also round trips, but it exposes binary noise: 0.6f would be written as 0.60000002384185791 instead of 0.600000024. There is a test guarding against that regression too, since exactness alone does not catch it. Readability cost, stated plainly because it is the reason this was not simply always correct: any value that is not exactly representable in binary now prints in full. 0.1 becomes 0.10000000000000001 and 9.81 becomes 9.8100000000000005, while 0.5 and 2.0 are unaffected. On the demo's system archive this changed the file size by less than one percent. The alternative that is both exact and tidy is to emit the shortest representation that round trips, which would render 0.1 as 0.1. std::to_chars does that directly, but its floating point overloads are not portable enough across the compilers Chrono supports, particularly older libstdc++ and libc++, so it is left as a possible follow-up rather than done here. ChArchiveOutASCII is deliberately untouched. It has no reader, so it cannot round trip and is a human-facing dump where 6 digits is a reasonable default. New test file utest_CH_archive_precision, in its own translation unit so that it does not collide with the additions PR #774 makes to utest_CH_archive. With the four source changes reverted, the four JSON and XML cases fail and the binary control still passes. All 11 core unit test binaries pass, including the pre-existing JSON, XML and Binary Fourbar round trips, which confirms the readers accept the longer values. demo_CH_archive still round-trips. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #778
ChArchiveOutJSON and ChArchiveOutXML wrote floating point values at the default ostream precision, which is 6 significant digits, while a double needs 17 to be recovered exactly. Saving a model to JSON or XML and loading it back therefore returned different numbers, silently. Measured, with the binary backend as a control since it writes raw bytes rather than printing them:
original JSON XML binary
0.33333333333333331 0.333333 0.333333 exact
12345.678901234567 12345.7 12345.7 exact
Note the second row: six significant digits is a relative budget, so the absolute error grows with magnitude, reaching about 0.022 there. Anyone using JSON or XML to checkpoint and restart a simulation was not restarting from where they stopped.
The fix raises the precision to max_digits10, which is the number of decimal digits that uniquely distinguishes every value of a type. It is set on the stream in the output archive's constructor and restored in the destructor, because the stream belongs to the caller and may be reused afterwards; std::cout, for one, should not be left permanently reconfigured. There is a test for that restoration.
The float overloads set 9 rather than inheriting the double's 17. Printing a float with eight digits more than it carries also round trips, but it exposes binary noise: 0.6f would be written as 0.60000002384185791 instead of 0.600000024. There is a test guarding against that regression too, since exactness alone does not catch it.
Readability cost, stated plainly because it is the reason this was not simply always correct: any value that is not exactly representable in binary now prints in full. 0.1 becomes 0.10000000000000001 and 9.81 becomes 9.8100000000000005, while 0.5 and 2.0 are unaffected. On the demo's system archive this changed the file size by less than one percent. The alternative that is both exact and tidy is to emit the shortest representation that round trips, which would render 0.1 as 0.1. std::to_chars does that directly, but its floating point overloads are not portable enough across the compilers Chrono supports, particularly older libstdc++ and libc++, so it is left as a possible follow-up rather than done here.
ChArchiveOutASCII is deliberately untouched. It has no reader, so it cannot round trip and is a human-facing dump where 6 digits is a reasonable default.
New test file utest_CH_archive_precision, in its own translation unit so that it does not collide with the additions PR #774 makes to utest_CH_archive. With the four source changes reverted, the four JSON and XML cases fail and the binary control still passes. All 11 core unit test binaries pass, including the pre-existing JSON, XML and Binary Fourbar round trips, which confirms the readers accept the longer values. demo_CH_archive still round-trips.