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

Make SolutionArray formatting consistent for different {fmt} versions #1547

Merged
merged 4 commits into from
Jul 18, 2023

Conversation

speth
Copy link
Member

@speth speth commented Jul 18, 2023

Changes proposed in this pull request

  • Store pure integer columns as integers
  • Be explicit about how to format floating point columns that contain only integers

With {fmt} 6.1.2, this changes the output of:

import cantera as ct
p = ct.Water()
arr = ct.SolutionArray(p, 12)
print(arr)

from

      T        D
0.0 300.0  996.633
1.0 300.0  996.633
2.0 300.0  996.633
3.0 300.0  996.633
4.0 300.0  996.633
..  ...      ...
7.0 300.0  996.633
8.0 300.0  996.633
9.0 300.0  996.633
10.0 300.0  996.633
11.0 300.0  996.633

[12 rows x 2 components; state='TD']

to

      T        D
0   300  996.633
1   300  996.633
2   300  996.633
3   300  996.633
4   300  996.633
..  ...      ...
7   300  996.633
8   300  996.633
9   300  996.633
10  300  996.633
11  300  996.633

[12 rows x 2 components; state='TD']

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

Closes #1526

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

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

Make it explicit that these should be printed as integers. Behavior of {fmt}
is different between 6.1.2, which always includes a decimal digit when the
input type is double if no other formatting specifier is provided, and later
versions which format integral doubles as doubles.

Fixes Cantera#1526
@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Merging #1547 (2782780) into main (a8a1c83) will decrease coverage by 0.03%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##             main    #1547      +/-   ##
==========================================
- Coverage   70.49%   70.46%   -0.03%     
==========================================
  Files         379      379              
  Lines       59091    59091              
  Branches    21228    21228              
==========================================
- Hits        41658    41641      -17     
- Misses      14353    14369      +16     
- Partials     3080     3081       +1     
Impacted Files Coverage Δ
src/base/SolutionArray.cpp 78.57% <71.42%> (ø)

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

ischoegl
ischoegl previously approved these changes Jul 18, 2023
Copy link
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on, @speth! ... I definitely appreciate that you were able to identify/resolve the issue.

Copy link
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

Should we set up ubuntu-multiple-pythons runners to use system_fmt=y, so this is covered by CI?

This extends the tested versions to include fmt 6.1.2 on Ubuntu 20.04
Copy link
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@ischoegl ischoegl merged commit ed628fc into Cantera:main Jul 18, 2023
@speth speth deleted the fix-1526-solnarray-formatting branch July 18, 2023 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

SolutionArray formatting tests fail with fmt 6.1.2
2 participants