Fix property updates in BinarySolutionTabulatedThermo #606
Conversation
Since IdealSolidSolnPhase::_updateThermo wasn't a virtual method, and the signatures didn't match (const vs non-const), calls to this method from IdealSolidSolnPhase weren't being overridden by BinarySolutionTabulatedThermo::_updateThermo as expected.
Codecov Report
@@ Coverage Diff @@
## master #606 +/- ##
==========================================
+ Coverage 68.5% 68.52% +0.01%
==========================================
Files 363 363
Lines 39962 39953 -9
==========================================
Hits 27376 27376
+ Misses 12586 12577 -9
Continue to review full report at Codecov.
|
|
Thanks, @speth - this logic sounds correct, and explains a few oddities I was seeing (but didn’t comprehend) when I was finalizing this PR. I’ll take a closer look in the next day or 2.
… On Mar 5, 2019, at 4:41 PM, Ray Speth ***@***.***> wrote:
Because IdealSolidSolnPhase::_updateThermo wasn't a virtual method, and the signature wasn't the same as BinarySolutionTabulatedThermo::_updateThermo (const vs non-const), calls to this method
from IdealSolidSolnPhase weren't being overridden by BinarySolutionTabulatedThermo::_updateThermo as expected.
After fixing this, there was still a problem in the case the temperature changed but the mole fractions were the same, where the tabulated correction to the enthalpy / entropy was being replaced by the pure species values. Now, we store the tabulated values and apply them every time.
You can view, comment on, or merge this pull request online at:
#606 <#606>
Commit Summary
[Thermo] Fix overriding of IdealSolidSolnPhase::_updateThermo
[Thermo] Fix BinarySolutionTabulatedThermo updates when only T changes
File Changes
M include/cantera/thermo/BinarySolutionTabulatedThermo.h <https://github.com/Cantera/cantera/pull/606/files#diff-0> (10)
M include/cantera/thermo/IdealSolidSolnPhase.h <https://github.com/Cantera/cantera/pull/606/files#diff-1> (2)
M src/thermo/BinarySolutionTabulatedThermo.cpp <https://github.com/Cantera/cantera/pull/606/files#diff-2> (51)
M test/thermo/BinarySolutionTabulatedThermo_Test.cpp <https://github.com/Cantera/cantera/pull/606/files#diff-3> (7)
Patch Links:
https://github.com/Cantera/cantera/pull/606.patch <https://github.com/Cantera/cantera/pull/606.patch>
https://github.com/Cantera/cantera/pull/606.diff <https://github.com/Cantera/cantera/pull/606.diff>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <#606>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEgvvAGyZI6KWwgErE1ECzppCMxsdapLks5vTwC7gaJpZM4bfu_8>.
|
| @@ -156,15 +156,15 @@ class BinarySolutionTabulatedThermo : public IdealSolidSolnPhase | |||
| size_t m_kk_tab; | |||
|
|
|||
| //! Current tabulated species mole fraction | |||
| double m_xlast; | |||
| mutable double m_xlast; | |||
|
|
|||
| //! Vector for storing tabulated thermo | |||
| vector_fp m_molefrac_tab; | |||
| vector_fp m_enthalpy_tab; | |||
| vector_fp m_entropy_tab; | |||
|
|
|||
| private: | |||
There was a problem hiding this comment.
Just for my own growth, can you explain what adding const after the method name does?
There was a problem hiding this comment.
A method with const at the end of the declaration cannot modify any member variables of the class, except those that are marked as mutable. See this SO answer for a more detailed explanation.
The fact that a non-const function does not override a const function which otherwise has the same signature is one of C++'s many subtle "gotchas".
| @@ -52,6 +52,9 @@ TEST_F(BinarySolutionTabulatedThermo_Test,interp_h) | |||
| { | |||
| set_defect_X(xmin + i*dx); | |||
| EXPECT_NEAR(expected_result[i], test_phase->enthalpy_mole(), 1.e-6); | |||
| // enthalpy is temperature-independent | |||
There was a problem hiding this comment.
This is correct for the cti file on which this test is built, but am I correct that it is not necessarily true of the phase model (i.e. the excess terms are added to the species "standard state" terms, which are calculated according to whatever is in the cti file?)
If I'm correct about that, there are two rather minor concerns:
- The test could easily be broken if the underlying cti file is updated w/ temperature-dependent species thermo. Thinking while I write, here, but this is actually a good thing, in that the cti probably shouldn't be changed in that way. It is good to be able to verify that temperature updates and composition updates are 'disentangled,' and the temperature-independent thermo provides a good pathway for this.
- Also a very minor concern, but someone reading this test suite to learn more about the class might get the wrong impression. Chances are vanishingly small that this would happen, particularly as there are other, better places to learn about the class. But it crossed my mind...
There was a problem hiding this comment.
Correct, that is a property of the example input file which sets the standard state to be h(T) = 0, not of the model as a whole. In general, I would expect changes to the test data files to cause some of the tests to fail -- if you can change the data files and all the tests still succeed, I'd argue that the tests aren't checking the right things.
I will update the comment to indicate why the enthalpy is temperature independent in this particular case, to avoid any confusion.
| m_tlast = tnow; | ||
| m_xlast = xnow; | ||
| } else if (m_tlast != tnow) { |
There was a problem hiding this comment.
Thanks for catching this. I know I thought through the logic when I was doing this, and am having a hard time figuring out if I actually thought this previous approach was satisfactory, or just forgot to go back and fix it.
Anyway, the new approach is a lot better, regardless.
In the case where temperature changes but the mole fractions are the same, we still need to apply the enthalpy and entropy offsets to the tabulated species.
The value of m_xlast should only be set to a valid value by _updateThermo, after it has calculated values for the tabulated enthalpy and entropy.
speth
force-pushed
the
fix-binary-tabulated
branch
from
cd91eeb
to
3c97fa0
Compare
Because
IdealSolidSolnPhase::_updateThermowasn't a virtual method, and the signature wasn't the same asBinarySolutionTabulatedThermo::_updateThermo(const vs non-const), calls to this methodfrom
IdealSolidSolnPhaseweren't being overridden byBinarySolutionTabulatedThermo::_updateThermoas expected.After fixing this, there was still a problem in the case the temperature changed but the mole fractions were the same, where the tabulated correction to the enthalpy / entropy was being replaced by the pure species values. Now, we store the tabulated values and apply them every time.