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

Introduce cantera/core.h for C++ Examples #1238

Merged
merged 4 commits into from
Jun 7, 2022

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Apr 5, 2022

Changes proposed in this pull request

  • Add relevant headers to Solution.h
  • Edit: Introduce cantera/core.h
  • Remove redundant headers from samples

Also:

  • Implement Solution::setTransport(const std::string&)

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

In many instances where newSolution handles the creation of a Cantera::Solution object, the main include needed in C++ would be

#include "cantera/core.h"

An alternative would be to introduce a new core.h header at the same level as thermo.h etc. (I.e. #include "cantera/core.h") ... Edit: this was adopted as an alternative to include common headers in Solution.h

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

@ischoegl ischoegl requested a review from a team April 5, 2022 12:08
Copy link
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

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

Per my comments below, IMO the preferred method to make newSolution more accessible to users would be to add a new header, say, solution.h.

samples/cxx/custom/custom.cpp Show resolved Hide resolved
include/cantera/base/Solution.h Outdated Show resolved Hide resolved
@ischoegl
Copy link
Member Author

ischoegl commented Apr 5, 2022

@bryanwweber / @speth ... thank you for the feedback.

Per my comments below, IMO the preferred method to make newSolution more accessible to users would be to add a new header, say, solution.h.

I had already eyed that idea (see comments on top). Ultimately, I believe

#include "cantera/core.h"

is the most intuitive nomenclature - which is now implemented (it combines all commonly used features of what I consider the core - Solution, ThermoPhase, Kinetics and TransportBase).

@ischoegl ischoegl changed the title Add essential includes to Solution.h Introduce cantera/core.h for C++ Examples Apr 5, 2022
@ischoegl ischoegl force-pushed the add-headers-to-solution branch 2 times, most recently from 32bf024 to c01eaed Compare April 5, 2022 17:01
Copy link
Member

@decaluwe decaluwe 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, this seems pretty non-controversial, and really cleans up the include headers in some of the examples!

Naive question: You have added core.h to the zerodim and onedim headers, but I noticed that there are no lines that get taken out (i.e. these files did not previously include any of the include files listed in core.h. Am I understanding that correctly? And if so, why is it necessary to include core.h now? (note: this is not criticism, more just me trying to understand).

Edit: Never mind, I think I figured it out. Including it here cleans up header statements for things like Combustor.cpp and kinetics1.cpp, which already include zerodim, et al. Is that correct?

include/cantera/core.h Outdated Show resolved Hide resolved
@ischoegl
Copy link
Member Author

ischoegl commented Apr 6, 2022

@decaluwe ... glad you like it!

Including it here cleans up header statements for things like Combustor.cpp and kinetics1.cpp, which already include zerodim, et al. Is that correct?

Correct. I view core.h as something that is needed for most Cantera applications. Beyond, zerodim and onedim are expansions that build on top of this.

@ischoegl
Copy link
Member Author

ischoegl commented Apr 6, 2022

Per discussion elsewhere, it may make sense to postpone this feature until after the 2.6 release. I will nevertheless keep the PR open as I think the feature is worthwhile.

@ischoegl ischoegl marked this pull request as draft April 6, 2022 03:16
@codecov
Copy link

codecov bot commented May 5, 2022

Codecov Report

Merging #1238 (31ab3d7) into main (c040b12) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1238      +/-   ##
==========================================
- Coverage   67.26%   67.19%   -0.08%     
==========================================
  Files         314      314              
  Lines       41887    41905      +18     
  Branches    16852    16862      +10     
==========================================
- Hits        28174    28156      -18     
- Misses      11483    11518      +35     
- Partials     2230     2231       +1     
Impacted Files Coverage Δ
include/cantera/base/Solution.h 93.33% <ø> (ø)
samples/cxx/openmp_ignition/openmp_ignition.cpp 84.84% <ø> (ø)
samples/cxx/flamespeed/flamespeed.cpp 91.89% <100.00%> (ø)
src/base/Solution.cpp 81.50% <100.00%> (+0.12%) ⬆️
include/cantera/kinetics/Reaction.h 80.95% <0.00%> (-13.50%) ⬇️
src/kinetics/Falloff.cpp 79.81% <0.00%> (-5.54%) ⬇️
include/cantera/kinetics/ReactionRate.h 80.00% <0.00%> (-4.10%) ⬇️
src/kinetics/InterfaceRate.cpp 88.00% <0.00%> (-3.08%) ⬇️
src/kinetics/PlogRate.cpp 91.48% <0.00%> (-3.02%) ⬇️
include/cantera/kinetics/Falloff.h 80.00% <0.00%> (-1.36%) ⬇️
... and 9 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@ischoegl ischoegl marked this pull request as ready for review May 5, 2022 03:25
@ischoegl
Copy link
Member Author

ischoegl commented May 5, 2022

This should be one of the easier PR's left over from ongoing work prior to 2.6. Now that we're on our way to 3.0, this should be good for a review.

I ended up adding Solution::setTransport(const std::string&), which I believe should be a helpful addition.

PS: I'm a little torn about including cantera/base/global.h in core.h, as most examples don't use it. At the same time, it is useful for logging, warnings, etc. (and it will be a lot shorter after removal of CTI/XML) Edit: removed.

@ischoegl
Copy link
Member Author

@speth / @bryanwweber … rebased and ready for review.

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. Overall, I think this looks good. In a way, this is a better thought out version of the Cantera.h header that used to exist before around Cantera 2.2.

samples/cxx/gas_transport/gas_transport.cpp Outdated Show resolved Hide resolved
samples/cxx/jacobian/derivative_speed.cpp Show resolved Hide resolved
src/base/Solution.cpp Outdated Show resolved Hide resolved
Co-authored-by: Ray Speth <speth@mit.edu>
@ischoegl
Copy link
Member Author

ischoegl commented Jun 7, 2022

@speth ... thanks for the review! I think this is ready ...

@ischoegl
Copy link
Member Author

ischoegl commented Jun 7, 2022

@bryanwweber ... do you approve?

@bryanwweber bryanwweber merged commit dceacdf into Cantera:main Jun 7, 2022
@ischoegl ischoegl mentioned this pull request Jun 8, 2022
5 tasks
@ischoegl ischoegl deleted the add-headers-to-solution branch June 8, 2022 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

4 participants