Cantera / cantera

Chemical kinetics, thermodynamics, and transport tool suite
https://cantera.org
Other
582 stars 342 forks source link

Limit {fmt} versions #1538

Closed ischoegl closed 12 months ago

ischoegl commented 12 months ago

Changes proposed in this pull request

Frequent API changes in the {fmt} library have led to numerous workarounds in fmt.h. At the same time, the library is small (especially after version 7.0) and can be easily included from a git submodule. Note that the latest released {fmt} version is 10.0.

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

Closes #1537, closes #1540

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

While this is was not an objective of the PR, the SolutionArray CSV writer speed was improved significantly. For main, the benchmark is:

In [1]: cd samples/python/onedim/
/Volumes/Data/work/GitHub/cantera/samples/python/onedim

In [2]: %run ion_free_flame.py
[...]

In [3]: %timeit f.save("test.csv", overwrite=True)
3.49 ms ± 116 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

Using the proposed PR (switch to {fmt}), this reduces to

In [3]: %timeit f.save("test.csv", overwrite=True)
1.95 ms ± 16 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

This could potentially be further improved by using version 7.1 fmt/os.h file output (1.52 ms) and/or the compile-time format string compilation FMT_COMPILE, but a major caveat is that the latter is not compatible with icc (beyond only being available after 7.0). Note that YAML output is significantly slower (bottlenecks are elsewhere), so the possible optimization is not worthwhile:

In [4]: %timeit f.save("test.h5", "test", overwrite=True)
6.81 ms ± 15.8 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

In [5]: %timeit f.save("test.yaml", "test", overwrite=True)
87.3 ms ± 122 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

Checklist

codecov[bot] commented 12 months ago

Codecov Report

Merging #1538 (db00fe1) into main (f89efe8) will decrease coverage by 0.03%. The diff coverage is 80.00%.

:exclamation: Current head db00fe1 differs from pull request most recent head 7bba1d2. Consider uploading reports for the commit 7bba1d2 to get more accurate results

@@            Coverage Diff             @@
##             main    #1538      +/-   ##
==========================================
- Coverage   70.50%   70.47%   -0.03%     
==========================================
  Files         376      376              
  Lines       59075    59081       +6     
  Branches    21219    21222       +3     
==========================================
- Hits        41649    41636      -13     
- Misses      14348    14365      +17     
- Partials     3078     3080       +2     
Impacted Files Coverage Δ
include/cantera/base/fmt.h 66.66% <ø> (ø)
src/base/SolutionArray.cpp 78.57% <76.19%> (-0.28%) :arrow_down:
interfaces/cython/cantera/onedim.py 82.63% <100.00%> (ø)
src/base/AnyMap.cpp 83.26% <100.00%> (ø)
src/oneD/Boundary1D.cpp 55.39% <100.00%> (ø)
src/oneD/StFlow.cpp 82.18% <100.00%> (ø)

... and 4 files with indirect coverage changes

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

speth commented 12 months ago

For Cantera 3.0, I would prefer to maintain compatibility with fmt 6.1.2, which is the version available on Ubuntu 20.04 LTS. While using the submodule is reasonably convenient for users compiling on their own systems, it is highly undesirable when building Debian packages.

ischoegl commented 12 months ago

For Cantera 3.0, I would prefer to maintain compatibility with fmt 6.1.2, which is the version available on Ubuntu 20.04 LTS. While using the submodule is reasonably convenient for users compiling on their own systems, it is highly undesirable when building Debian packages.

Alright. I currently don't have a machine to test 6.1.2 (Apple silicon conda refuses to install fmt < 7.1), but I believe the PR by itself is worthwhile even if it does not resolve #1526 (it does fix two other unrelated issues). I set the minimum fmt version to 6.1.2, and removed code that assumed fmt 7.1 capabilities. I'd appreciate if we could merge this before tackling #1526?

PS: same on Windows 😠

Could not solve for environment specs
The following package could not be installed
└─ fmt 6.1.2**  does not exist (perhaps a typo or a missing channel).
ischoegl commented 12 months ago

@speth ... thanks for your comments; everything should be taken care of.

ischoegl commented 12 months ago

With this one, I think we still need a python-version list if we want it to run all the combinations of python-version and vs-toolset. In the latest CI run, this only ran (VS 14.1, Python 3.11, fmt 7.1) and (VS 14.3, Python 3.11, fmt 7.1).

Fixed!