coverage-dependent surface phase thermodynamics

[jongyoonbae]

Changes proposed in this pull request

• CoverageDependentSurfPhase class allows to incorporate coverage-dependency into the SurfPhase thermodynamics. Below is the list of new features introduced in the CoverageDependentSurfPhase.

1. Coverage-dependent enthalpy, entropy, and consequently chemical potential calculations method with (1). Linear model, (2). Piecewise linear model, (3). Interpolative model, and (4). Polynomial model up to 4th order.
2. Coverage-dependent heat capacity calculation method.
3. Optional user-defined reference coverage specification.
4. Optional user-defined coverage-dependent parameter's unit specification.
5. Updated coverage-dependent thermodynamic calculation methods for (1). Standard state properties, (2). partial molar properties, (3). Solution properties.
6. Scons test routine to test out all of the new/modified methods.

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

Closes #

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

[codecov]

Codecov Report

Merging #1135 (b353fe8) into main (0c5fc1d) will increase coverage by 0.10%.
The diff coverage is 82.67%.

❗ Current head b353fe8 differs from pull request most recent head 5c602e4. Consider uploading reports for the commit 5c602e4 to get more accurate results

@@            Coverage Diff             @@
##             main    #1135      +/-   ##
==========================================
+ Coverage   69.86%   69.96%   +0.10%     
==========================================
  Files         373      375       +2     
  Lines       56684    57035     +351     
  Branches    18883    19053     +170     
==========================================
+ Hits        39600    39903     +303     
- Misses      14571    14586      +15     
- Partials     2513     2546      +33     

Impacted Files	Coverage Δ
include/cantera/thermo/SurfPhase.h	100.00% <ø> (ø)
...nclude/cantera/thermo/CoverageDependentSurfPhase.h	68.00% <68.00%> (ø)
src/thermo/CoverageDependentSurfPhase.cpp	86.54% <86.54%> (ø)
interfaces/cython/cantera/thermo.pyx	92.35% <100.00%> (ø)
src/thermo/ThermoFactory.cpp	74.15% <100.00%> (+0.10%)	⬆️

... and 2 files with indirect coverage changes

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

[bryanwweber]

Hi @jongyoonbae! Nice work here. A few clean up comments for you, before the real work begins 😊

[speth]

Hi @jongyoonbae, this is looking very interesting. I think the main structure of this looks pretty good, and most of the changes are self-contained. Besides the specific comments below, I had a few general suggestions / questions:

• Documentation of the input format needs to be added to doc/sphinx/yaml/phases.rst and doc/sphinx/yaml/species.rst.
• Do you have a Python example that could be added? Right now, this feature is pretty much undiscoverable.
• I think it would be good to have some tests that more transparently show that the model is working as expected, for example cases where you can use simple explicit calculations to show that the values are what you expect. Another useful category of tests that you could do would be to calculate some properties at the limits where these coverage-dependent effects are zero, in comparison to a phase created using the SurfPhase class.
• Please check your code for end-of-line whitespace and remove it. Your IDE probably has an option / plugin that can either highlight it or automatically remove it for you.
• Please fix some of the spelling errors in the comments. If you're using VS Code, it should highlight most of them for you.
• Please do an interactive rebase to eliminate the addition of the compiled file test_problems/VPsilane_test/VPsilane_test

[bryanwweber]

@jongyoonbae Do you have any updates on this PR?

[jongyoonbae]

@jongyoonbae Do you have any updates on this PR?

I made some changes but not for all the comments. I will push those by the end of this week. Thanks!

[bryanwweber]

@jongyoonbae is this ready for another round of review?

[jongyoonbae]

@jongyoonbae is this ready for another round of review?
Yes it is! I still need to work on the python example and the sphinx documentation but the main code block is revised.

[speth]

Hi @jongyoonbae, thanks for the updates so far. I have some additional suggestions for
this PR based on the updated version. Besides the in-line comments, there are three general comments I would make:

• Please add a Doxygen docstring for the CoverageDependentSurfPhase class, build the Doxygen docs (run scons doxygen) and take a look at the output so you can fix the issues that should be visible. In particular, there are numerous issues with formatting of mathematical symbols.

• I think we need some additional clarity about the definition of what "standard state" means for this phase (yes, I know, this perpetual headache). In particular, are the following correct:

  • The standard state is defined as being at the current vector of coverages
  • The difference between the "reference state" and the "standard state" properties is the coverage-dependent term
  • The partial molar enthalpies and heat capacities are equal their standard state values
  • the partial molar entropy differs from the standard state value only by R*log(θ/θ_ref)
    I'd appreciate it if @decaluwe and/or @cfgoldsmith could weigh in here.

• To correctly enable YAML serialization, you will need to implement the methods void getSpeciesParameters(const std::string& name, AnyMap& speciesNode) and void getParameters(AnyMap& phaseNode) const. You should also add a corresponding "roundtrip" test case to thermoToYaml.cpp.

Furthermore, some of the comments I made previously have not been addressed. Please fix these issues:

• Please enable spellcheck in your editor and take a look at some of its suggestions. There are quite a few typos in the docstrings, such as (for starters) "thermodyanmic", "caclulated", "caculated", "dependece", "logarithimic", "Asumming", "capapcities".
• Please do an interactive rebase to eliminate the addition of the compiled file test_problems/VPsilane_test/VPsilane_test. If you don't know how to do this, I can do it for you and force push to this branch, at which point you will need to fetch this branch again.

From the previous review, the following also still need to be addressed, but you already mentioned that you're still working on these. I'm repeating them just so they don't get lost:

• The YAML input fields for this phase (I guess this is just reference-state-coverage?) and the coverage-dependencies section of the species entries need to be documented in doc/sphinx/yaml/phases.rst and doc/sphinx/yaml/species.rst, respectively.
• Please add a Python example so this feature is more discoverable.

[speth]

What does it mean for the standard state enthalpy to be dependent on the coverages? Even by Cantera's odd definition of "standard state", I don't understand this. (paging @cfgoldsmith, @decaluwe).

[jongyoonbae]

Hi @jongyoonbae, thanks for the updates so far. I have some additional suggestions for this PR based on the updated version. Besides the in-line comments, there are three general comments I would make:

• Please add a Doxygen docstring for the CoverageDependentSurfPhase class, build the Doxygen docs (run scons doxygen) and take a look at the output so you can fix the issues that should be visible. In particular, there are numerous issues with formatting of mathematical symbols.

• I think we need some additional clarity about the definition of what "standard state" means for this phase (yes, I know, this perpetual headache). In particular, are the following correct:

  • The standard state is defined as being at the current vector of coverages
  • The difference between the "reference state" and the "standard state" properties is the coverage-dependent term
  • The partial molar enthalpies and heat capacities are equal their standard state values
  • the partial molar entropy differs from the standard state value only by R*log(θ/θ_ref)
    I'd appreciate it if @decaluwe and/or @cfgoldsmith could weigh in here.

• To correctly enable YAML serialization, you will need to implement the methods void getSpeciesParameters(const std::string& name, AnyMap& speciesNode) and void getParameters(AnyMap& phaseNode) const. You should also add a corresponding "roundtrip" test case to thermoToYaml.cpp.

Furthermore, some of the comments I made previously have not been addressed. Please fix these issues:

• Please enable spellcheck in your editor and take a look at some of its suggestions. There are quite a few typos in the docstrings, such as (for starters) "thermodyanmic", "caclulated", "caculated", "dependece", "logarithimic", "Asumming", "capapcities".
• Please do an interactive rebase to eliminate the addition of the compiled file test_problems/VPsilane_test/VPsilane_test. If you don't know how to do this, I can do it for you and force push to this branch, at which point you will need to fetch this branch again.

From the previous review, the following also still need to be addressed, but you already mentioned that you're still working on these. I'm repeating them just so they don't get lost:

• The YAML input fields for this phase (I guess this is just reference-state-coverage?) and the coverage-dependencies section of the species entries need to be documented in doc/sphinx/yaml/phases.rst and doc/sphinx/yaml/species.rst, respectively.
• Please add a Python example so this feature is more discoverable.

Could you please do this part? "Please do an interactive rebase to eliminate the addition of the compiled file test_problems/VPsilane_test/VPsilane_test."
I will fetch once it's done.

[speth]

@jongyoonbae - I've eliminated the addition of the VPsilane_test binary from the commit history and pushed that to this branch. If you fetch and reset your local branch, that should bring it up to date.

[speth]

Hi @jongyoonbae, thank you for the updates. There is a small issue with the commit history that would be worth cleaning up. Can you eliminate the unnecessary "merge" commit? I think you can do this by just running

git rebase 3cd8e5e74
git push --force-with-lease

I think this should also eliminate the duplicated commit with the message "Add ThermoYamlRoundtrip test and python example" (where I think the only difference is that you corrected the typo in the commit message). This might also be a good time to rebase onto the current main branch.

[jongyoonbae]

Hi @jongyoonbae, thank you for the updates. There is a small issue with the commit history that would be worth cleaning up. Can you eliminate the unnecessary "merge" commit? I think you can do this by just running

git rebase 3cd8e5e74
git push --force-with-lease

I think this should also eliminate the duplicated commit with the message "Add ThermoYamlRoundtrip test and python example" (where I think the only difference is that you corrected the typo in the commit message). This might also be a good time to rebase onto the current main branch.

Thank you @speth for the great tips! I rebased to clean up the commit history and rebased this branch onto the main branch.

[speth]

Thanks for the updates, @jongyoonbae. I think this is generally looking very good. I appreciate the consolidation of the different options for specifying the model parameters. Also, the plots produced by the Python example are a very nice and do a good job of showing the effects of this model.

Most of my comments here are pretty minor cleanup issues that should be easy to address.

[speth]

Thanks for the quick updates, @jongyoonbae. I think this is good to go. I just pushed an update to squash a couple of commits and clean up a few commit messages. This should be good to merge as soon as the CI tests finish.

[speth]

Congratulations, @jongyoonbae, on putting Cantera over the milestone of 10,000 commits with this pull request (specifically, 2a8d54e). 🥳