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

[thermo] fix plasma enthalpy_mole #1323

Merged
merged 7 commits into from
Jan 15, 2023
Merged

Conversation

BangShiuh
Copy link
Contributor

@BangShiuh BangShiuh commented Jun 7, 2022

Fix issue #1306

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 Jun 7, 2022

Codecov Report

Merging #1323 (b940d27) into main (957dd24) will increase coverage by 0.02%.
The diff coverage is 98.76%.

@@            Coverage Diff             @@
##             main    #1323      +/-   ##
==========================================
+ Coverage   70.79%   70.82%   +0.02%     
==========================================
  Files         373      373              
  Lines       53873    53949      +76     
  Branches    17579    17587       +8     
==========================================
+ Hits        38141    38208      +67     
- Misses      13331    13339       +8     
- Partials     2401     2402       +1     
Impacted Files Coverage Δ
interfaces/cython/cantera/thermo.pyx 92.07% <66.66%> (-0.11%) ⬇️
include/cantera/thermo/PlasmaPhase.h 81.39% <100.00%> (+9.96%) ⬆️
src/thermo/IdealGasPhase.cpp 92.99% <100.00%> (-0.14%) ⬇️
src/thermo/PlasmaPhase.cpp 77.91% <100.00%> (+7.52%) ⬆️
...terfaces/dotnet/Cantera/src/Interop/InteropUtil.cs 60.75% <0.00%> (-8.87%) ⬇️
src/base/stringUtils.cpp 79.03% <0.00%> (-0.81%) ⬇️

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

@BangShiuh BangShiuh force-pushed the fix-electron-thermo branch 3 times, most recently from 0f7a04c to 4c7202e Compare June 8, 2022 21:03
@ischoegl
Copy link
Member

ischoegl commented Oct 1, 2022

@BangShiuh … do you have any updates on this PR?

@BangShiuh
Copy link
Contributor Author

@ischoegl There are some issues defining the thermodynamic properties of plasma. I am still working on it.

@BangShiuh BangShiuh force-pushed the fix-electron-thermo branch 2 times, most recently from ebcc7a8 to e165f0d Compare October 15, 2022 22:16
@BangShiuh
Copy link
Contributor Author

@speth The consistency test is very helpful to get the thermodynamics correct. However, I still failed for case gk_eq_hk_minus_T_sk. Also in the case hk_eq_uk_plus_P_vk, I use RTe instead of p * vk[k] because I am not sure how to define a partial molar volume for electrons.

@speth
Copy link
Member

speth commented Oct 16, 2022

For gk_eq_hk_minus_T_sk, I think you could mark that as an "expected failure" as the test doesn't know that it should use the electron temperature for the electron species in that summation. If you add a test in the PlasmaPhase specific tests that checks this identity with the right temperature being used for each species, I think that would be an appropriate alternative.

In the second case, is there a problem with using the same definition of the partial molar volume as used for the gas phase species (which is just the inverse of the mixture molar density, for all species)? I guess the question becomes whether the distinct electron temperature is being considered in the equation of state or not.

@BangShiuh BangShiuh marked this pull request as ready for review November 2, 2022 19:43
@BangShiuh
Copy link
Contributor Author

@speth Any thought on this PR?

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 for the updates to this, @BangShiuh. The main parts of the implementation all look reasonable to me. I just had a few questions / suggestions for your consideration.

include/cantera/thermo/PlasmaPhase.h Outdated Show resolved Hide resolved
src/thermo/PlasmaPhase.cpp Outdated Show resolved Hide resolved
include/cantera/thermo/ThermoPhase.h Outdated Show resolved Hide resolved
interfaces/cython/cantera/thermo.pxd Outdated Show resolved Hide resolved
src/thermo/PlasmaPhase.cpp Outdated Show resolved Hide resolved
Comment on lines 519 to 527
roundtrip("oxygen-plasma.yaml", "isotropic-electron-energy-plasma");
compareThermo(800, 2*OneAtm);
auto origPlasma = std::dynamic_pointer_cast<PlasmaPhase>(original);
Copy link
Member

Choose a reason for hiding this comment

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

Why does this comparison have to be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mixture thermal properties like enthalpy_mole() are not applicable for PlasmaPhase because it has gas and electron temperature.

Copy link
Member

Choose a reason for hiding this comment

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

The initial point of this PR was to provide a working implementation of enthalpy_mole, though. I pushed an alternative that selectively disables the two methods called by compareThermo that are not implemented in PlasmaPhase (cp_mole and entropy_mole).

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.

This looks good to me. @ischoegl - I'd like to leave final approval of this to you, since I stuck a commit of my own in here and don't want to be just approving my own code, even if it is relatively minor.

@ischoegl ischoegl merged commit 1fa20b7 into Cantera:main Jan 15, 2023
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.

3 participants