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

Assorted C++ / SCons cleanup #1574

Merged
merged 9 commits into from
Aug 9, 2023
Merged

Assorted C++ / SCons cleanup #1574

merged 9 commits into from
Aug 9, 2023

Conversation

speth
Copy link
Member

@speth speth commented Aug 8, 2023

Changes proposed in this pull request

  • Update check for Boost to specify 1.70 as minimum version
  • Use FMT_HEADER_ONLY only for platforms and fmt versions where necessary (revises fix for MSVC failures with fmt 7.1 #1540)
  • Fix reporting Eigen version during build process
  • Clean up spacing around angle brackets in templates, e.g. change vector<vector<double> > to vector<vector<double>>
  • Deprecate unused begin() / end() methods of matrix classes
  • Remove vestigial ReactorSurface data from WallBase
  • Keep conda build environment path out of generated cantera.pc
  • Skip pint-based tests for pint<0.17.0, where the test helpers aren't available

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 Aug 8, 2023

Codecov Report

Merging #1574 (c1eec22) into main (25851e7) will decrease coverage by 0.02%.
The diff coverage is 59.57%.

❗ Current head c1eec22 differs from pull request most recent head 3b30d09. Consider uploading reports for the commit 3b30d09 to get more accurate results

@@            Coverage Diff             @@
##             main    #1574      +/-   ##
==========================================
- Coverage   70.53%   70.52%   -0.02%     
==========================================
  Files         379      379              
  Lines       59110    59122      +12     
  Branches    21232    21238       +6     
==========================================
+ Hits        41695    41697       +2     
- Misses      14340    14352      +12     
+ Partials     3075     3073       -2     
Files Changed Coverage Δ
include/cantera/base/AnyMap.h 80.00% <ø> (ø)
include/cantera/base/Array.h 96.00% <ø> (+13.24%) ⬆️
include/cantera/base/ValueCache.h 100.00% <ø> (ø)
include/cantera/kinetics/InterfaceKinetics.h 87.50% <ø> (ø)
include/cantera/kinetics/Kinetics.h 32.66% <ø> (ø)
include/cantera/kinetics/ReactionPath.h 67.44% <ø> (ø)
include/cantera/kinetics/ThirdBodyCalc.h 98.38% <ø> (ø)
include/cantera/numerics/BandMatrix.h 0.00% <ø> (ø)
include/cantera/numerics/GeneralMatrix.h 15.38% <ø> (ø)
...nclude/cantera/thermo/CoverageDependentSurfPhase.h 100.00% <ø> (+32.00%) ⬆️
... and 10 more

... and 1 file with indirect coverage changes

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

@speth speth changed the title Assorted C++ cleanup Assorted C++ / SCons cleanup Aug 9, 2023
@speth speth marked this pull request as ready for review August 9, 2023 13:22
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.

Looks good to me!

@ischoegl ischoegl merged commit 4b30fa7 into Cantera:main Aug 9, 2023
40 of 42 checks passed
@ischoegl
Copy link
Member

ischoegl commented Aug 9, 2023

PS: Regarding the removal of ReactorSurface from Wall it may be worth revisiting in the future.

@speth
Copy link
Member Author

speth commented Aug 9, 2023

PS: Regarding the removal of ReactorSurface from Wall it may be worth revisiting in the future.

I don't think so. The only thing that Wall and ReactorSurface have in common is that they both have an area. Wall objects represent interactions between two reactors, while ReactorSurface objects represent heterogeneous kinetics in a single reactor. Separating these capabilities led to a significant simplification in setting up networks with ReactorSurfaces, since you don't need to define a separate Reactor or Reservoir to insert the wall between just to be able to add a reacting surface.

@speth speth deleted the code-cleanup branch August 9, 2023 20:12
@ischoegl
Copy link
Member

ischoegl commented Aug 9, 2023

I guess I misspoke: it's actually WallBase I had in mind, see doxygen wallGroup. While this may still not make sense, there's also this old issue that we may be able to resolve at some point ... Cantera/enhancements#92

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.

None yet

2 participants