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

Improve YAML formatting #1133

Merged
merged 5 commits into from
Nov 20, 2021
Merged

Improve YAML formatting #1133

merged 5 commits into from
Nov 20, 2021

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Oct 26, 2021

Leverage fmt::format to appropriately round values.

Changes proposed in this pull request

  • Update rounding rules for double emitter
  • Implement multi-line strings

If applicable, fill in the issue number this pull request is fixing

Closes #1128 (together with improved formatting documentation introduced in #1112)

If applicable, provide an example illustrating new features this pull request is introducing

(A) Rounding of floating point values

Ensure that floating point values are properly rounded, e.g.

<       - {P: 10.0, A: 1.2866e+47, b: -9.0246, Ea: 2.00263761648836e+04}
<       - {P: 100.0, A: 5.9632e+56, b: -11.529, Ea: 2.64691461742217e+04}
---
>       - {P: 9.99999999999999, A: 1.2866e+47, b: -9.0246,
>       Ea: 2.00263761648836e+04}
>       - {P: 99.9999999999999, A: 5.9632e+56, b: -11.529,
>       Ea: 2.64691461742217e+04}

(B) Serialization of lines spanning multiple lines (aka 'Literal' or folded strings)

Strings that contain '\n' are written as multiple lines, e.g. "spam\nand\neggs" becomes:

literal-string: |
  spam
  and
  eggs

which leverages YAML::Literal. At the moment, there doesn't appear to be a way to skip the trailing \n in the equivalent string that is written (i.e. there is no |- rather than | in yaml-cpp for the first line as far as I can tell).

FWIW, here's a demo illustrating different literal/folded multiline string versions in YAML. yaml-cpp appears to only support one version.

Checklist

  • The pull request includes a clear description of this code change
  • Commit messages have short titles and reference relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • Style & formatting of contributed code follows contributing guidelines
  • The pull request is ready for review

@codecov
Copy link

codecov bot commented Oct 26, 2021

Codecov Report

Merging #1133 (1478bf1) into main (4570f3e) will increase coverage by 0.02%.
The diff coverage is 97.40%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1133      +/-   ##
==========================================
+ Coverage   73.49%   73.52%   +0.02%     
==========================================
  Files         365      365              
  Lines       48187    48242      +55     
==========================================
+ Hits        35417    35470      +53     
- Misses      12770    12772       +2     
Impacted Files Coverage Δ
src/base/AnyMap.cpp 89.83% <95.91%> (+0.07%) ⬆️
test/general/test_serialization.cpp 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4570f3e...1478bf1. Read the comment docs.

@ischoegl ischoegl force-pushed the update-yaml-emitters branch 3 times, most recently from c7a6eca to 3e35f46 Compare October 26, 2021 17:24
@ischoegl ischoegl marked this pull request as ready for review October 26, 2021 17:30
@ischoegl ischoegl requested a review from a team October 26, 2021 18:08
Use fmt::format to detect rounded values.
Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @ischoegl. I think these changes to the output formatting generally look very good. However, I did notice that there were a few cases where the output is now outputting trailing zeros in some circumstances, and I don't think this is expected based on the comments in the formatDouble method. A couple of examples can be seen in the test suite. In test/work/generated-ptcombust.yaml, I see:

    site-density: 1.629771953878800e+13

and in test/work/generated-h2o2-from-xml.yaml, we have:

    rate-constant: {A: 3.57e+04, b: 2.4, Ea: -1061.79321568240}

I'm not sure exactly how this is happening, but perhaps it's worth a few tests that this formatter is actually generating the desired output (recognizing that the lack of tests for this method is an oversight on my fault to begin with 😬).

src/base/AnyMap.cpp Outdated Show resolved Hide resolved
@ischoegl
Copy link
Member Author

ischoegl commented Nov 19, 2021

@speth ... thank you for the review!

Interesting find about the double trailing zeros. It is not entirely surprising as I was very careful to not round and inadvertently introduce error. I am now removing double '00' but also added the following explanation to formatDouble:

string formatDouble(double x, long int precision)
{
    // This function ensures that trailing zeros resulting from round-off error
    // are removed. Values are only rounded if at least three digits are removed,
    // or the displayed value has multiple trailing zeros.

This choice is partially aesthetic (e.g. long blocks of values), but also tries to stay on the safe side while catching compounded round-off error (not just trailing 001's and 999's).

I also added the requested unit test. It's somewhat tricky as the exact number representation may be impacted by the architecture, but I believe what I implemented should have the desired output. One curious finding was that removing every trailing zero breaks the test suite (i.e. removing a single zero may change the actual value!). Removing double trailing zeros appears to be safe.

@ischoegl ischoegl requested a review from speth November 20, 2021 00:36
Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @ischoegl. This looks good to me.

@speth speth merged commit 4b4eee1 into Cantera:main Nov 20, 2021
@ischoegl ischoegl deleted the update-yaml-emitters branch November 22, 2021 18:16
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

Successfully merging this pull request may close these issues.

Improve YAML output formatting
2 participants