Deprecate redundant ThermoPhase classes #787

speth · 2020-01-04T00:31:23Z

There is a clear use-case for this code change
The commit message has a short title & references relevant issues
Build passes (scons build & scons test) and unit tests address code coverage

Changes proposed in this pull request

Deprecate the "gas" mode of IdealSolnGasVPSS - this mode duplicates the more widely-used IdealGasPhase class.
Deprecate FixedChemPotSSTP - this class can be easily replaced using the StoichSubstance class with a ConstCpPoly species thermo object.

codecov · 2020-01-04T01:01:54Z

Codecov Report

Merging #787 into master will decrease coverage by 0.02%.
The diff coverage is 87.09%.

@@            Coverage Diff             @@
##           master     #787      +/-   ##
==========================================
- Coverage   71.45%   71.43%   -0.03%     
==========================================
  Files         372      372              
  Lines       43522    43547      +25     
==========================================
+ Hits        31098    31106       +8     
- Misses      12424    12441      +17

Impacted Files	Coverage Δ
src/thermo/FixedChemPotSSTP.cpp	`55.00% <50.00%> (-11.97%)`	⬇️
include/cantera/thermo/IdealSolnGasVPSS.h	`91.66% <100.00%> (+2.77%)`	⬆️
test/thermo/phaseConstructors.cpp	`100.00% <100.00%> (ø)`
test/thermo/thermoFromYaml.cpp	`100.00% <100.00%> (ø)`
..._problems/VCSnonideal/LatticeSolid_LiSi/latsol.cpp	`93.18% <100.00%> (+0.49%)`	⬆️
test_problems/VPsilane_test/silane_equil.cpp	`69.23% <100.00%> (+2.56%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a9f637...d2608d9. Read the comment docs.

decaluwe · 2020-04-04T03:58:40Z

src/thermo/FixedChemPotSSTP.cpp

    importPhase(xmlphase, this);
 }

 FixedChemPotSSTP::FixedChemPotSSTP(const std::string& Ename, doublereal val) :
    chemPot_(0.0)
 {
+    warn_deprecated("classFixedChemPotSSTP", "To be removed after Cantera 2.5. "
+        "Use class StoichSubstance with a Mu0Poly (piecewise-Gibbs) species "
+        "thermo model");


Why is this deprecated message different from the others?

Not a copy paste error, but still incorrect. I had initially thought that Mu0Poly was the species thermo model that could be used here, but the constant-cp model is actually the one that has the same behavior as this class.

decaluwe · 2020-04-04T04:05:48Z

Hi @speth - sorry for the time it took me to look at this. I feel like we discussed this at the steering committee meeting, and from what I remember these classes are indeed largely redundant.

The specific changes all look good to me.

I noticed one place where the deprecation message was different from the others, possibly a copy-paste error? Noted above.
I largely ignored close examination of the test results, but did notice that the internal energy values, for example, differed between the old and new implementations. Not sure whether this might matter for a user, but I suppose a FixedChemPotential model is not discriminating based on internal energy, anyway.
Out of curiosity, what does make_deprecation_warnings_fatal do?

This model can easily be replaced with StoichSustance using ConstCpPoly for the species thermo. The changed values in the LiSi regression tests suggest that there are some implementation errors in the FixedChemPotSSTP class which will be automatically resolved by eliminating it.

speth · 2020-04-04T19:01:01Z

The internal energy values change because the FixedChemPotSSTP class didn't actually set the density, leaving it at the nonsense default value of 0.001 kg/m^3. The StoichSubstance class in contrast actually uses the specified density value. For both classes, the enthalpy is the same but since the internal energy is calculated as u = h - p/rho, it has to change.

make_deprecation_warnings_fatal changes deprecation warnings into exceptions. The purpose of using this is so that use of deprecated methods in the test suite causes the test to fail, rather than generating warnings that might never be noticed.

decaluwe

This looks good to me. A few general questions, but I'm guessing there are no real issues, here.

So a general question: am I correct that VPSS stands for "variable pressure standard state" (i.e. the standard state depends on both temperature and pressure? If so,

a. This seems coincidentally relevant to issue IdealGasPhase getStandardChemPotentials #841 that I added yesterday (i.e. this PR provides insight on that issue, not vice-versa), and

b. I wonder if there is any reason a user would want to have this as a separate option? The actual phase behavior between IdealGasPhase and the gas option for IdealSolnGasVPSS would be exactly the same, it seems; is there any benefit to the user to having the standard state be pressure sensitive? Would they ever want to interrogate that property? My guess:
- No.
- Even if so, the benefit and degree of interest in this capability would be so minimal that this PR is still the right move.
Now that I'm thinking about this a little more, are the solid and liquid options here also redundant? It seems like they might duplicate IdealSolidSolution? Just a hunch. Side note: I'll also note that there's no real reason to have the Solid as part of this latter class's name. I've found it just as useful for liquid solutions. But I digress...

Assuming the answers to the above are what I think they are, this looks like it's ready to go.

Thanks!

speth · 2020-04-05T15:08:47Z

So a general question: am I correct that VPSS stands for "variable pressure standard state"

Yes. I think this name is a bit of a misnomer, however, compared to what the class actually does.

a. This seems coincidentally relevant to issue #841 that I added yesterday (i.e. this PR provides insight on that issue, not vice-versa), and

Let's deal with that elsewhere.

b. I wonder if there is any reason a user would want to have this as a separate option? The actual phase behavior between IdealGasPhase and the gas option for IdealSolnGasVPSS would be exactly the same, it seems; is there any benefit to the user to having the standard state be pressure sensitive? Would they ever want to interrogate that property?

Well, as far as I can tell, both IdealGasPhase and IdealSolnGasVPSS return the same values (to within rounding error) even for the standard state properties, such as getGibbs_RT. So I'm not sure that there is an observable difference between these classes.

2. Now that I'm thinking about this a little more, are the solid and liquid options here also redundant? It seems like they might duplicate IdealSolidSolution? Just a hunch. Side note: I'll also note that there's no real reason to have the Solid as part of this latter class's name. I've found it just as useful for liquid solutions.

The IdealSolidSolution class isn't a full replacement for IdealSolnGasVPSS in "soln" mode because you can't use the PDSS_SSVol (density as a function of temperature) species thermo model with IdealSolidSolution. So what I was thinking is that longer term, it might make sense to rename IdealSolnGasVPSS to IdealCondensed and deprecate IdealSolidSolution in favor of that.

bryanwweber · 2020-04-05T16:31:51Z

It seems like this PR reverted to SUNDIALS 5.0.0. Was that intentional?

speth · 2020-04-05T17:19:35Z

No, obviously not. Fix pushed directly to master (5ae37ad).

speth requested a review from decaluwe January 11, 2020 17:53

speth mentioned this pull request Feb 24, 2020

YAML format documentation Cantera/cantera-website#91

Merged

decaluwe reviewed Apr 4, 2020

View reviewed changes

speth added 2 commits April 4, 2020 14:48

[Thermo] Deprecate "gas" mode of IdealSolnGasVPSS

149136d

speth force-pushed the deprecate-redundant-thermo branch from 9ba7926 to d2608d9 Compare April 4, 2020 19:00

speth requested a review from decaluwe April 4, 2020 19:01

decaluwe approved these changes Apr 5, 2020

View reviewed changes

speth merged commit 15b9b33 into Cantera:master Apr 5, 2020

speth deleted the deprecate-redundant-thermo branch April 5, 2020 17:19

bryanwweber referenced this pull request Apr 14, 2020

Revert accidental change of Sundials submodule version

5ae37ad

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate redundant ThermoPhase classes #787

Deprecate redundant ThermoPhase classes #787

speth commented Jan 4, 2020

codecov bot commented Jan 4, 2020 •

edited

Loading

decaluwe Apr 4, 2020

speth Apr 4, 2020

decaluwe commented Apr 4, 2020 •

edited

Loading

speth commented Apr 4, 2020

decaluwe left a comment •

edited

Loading

speth commented Apr 5, 2020 •

edited

Loading

bryanwweber commented Apr 5, 2020

speth commented Apr 5, 2020

Deprecate redundant ThermoPhase classes #787

Deprecate redundant ThermoPhase classes #787

Conversation

speth commented Jan 4, 2020

codecov bot commented Jan 4, 2020 • edited Loading

Codecov Report

decaluwe Apr 4, 2020

Choose a reason for hiding this comment

speth Apr 4, 2020

Choose a reason for hiding this comment

decaluwe commented Apr 4, 2020 • edited Loading

speth commented Apr 4, 2020

decaluwe left a comment • edited Loading

Choose a reason for hiding this comment

speth commented Apr 5, 2020 • edited Loading

bryanwweber commented Apr 5, 2020

speth commented Apr 5, 2020

codecov bot commented Jan 4, 2020 •

edited

Loading

decaluwe commented Apr 4, 2020 •

edited

Loading

decaluwe left a comment •

edited

Loading

speth commented Apr 5, 2020 •

edited

Loading