-
-
Notifications
You must be signed in to change notification settings - Fork 349
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
Extend BinarySolutionTabulatedThermo by tabulated partial molar volumes #1072
Extend BinarySolutionTabulatedThermo by tabulated partial molar volumes #1072
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Thank you for submitting these changes, the functionality looks like it will be useful. I have several formatting/style comments, if you can please implement them while the technical review is ongoing. Can you also please add a test for this new functionality? If any of my comments are not clear, please post a comment and ask about it, I am more than happy to help. Thank you!
void getIntegratedPartialMolarVolumes(doublereal* vbar); | ||
void getPartialMolarVolumes(doublereal* vbar) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid using doublereal
in new code, preferring the standard double
instead.
protected: | ||
//! If the compositions have changed, update the tabulated thermo lookup | ||
virtual void compositionChanged(); | ||
|
||
//! Species thermodynamics interpolation functions | ||
std::pair<double,double> interpolate(double x) const; | ||
double interpolate(double x,const vector_fp& molefrac,const vector_fp& inputData) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function definitions should please have one space after a comma. I'm not sure exactly how long this line is, but we prefer to keep line lengths to 88 characters or less, if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other words:
double interpolate(double x,const vector_fp& molefrac,const vector_fp& inputData) const; | |
double interpolate(double x, const vector_fp& molefrac, | |
const vector_fp& inputData) const; |
There is a similar issue on line 162 (next declaration)
@@ -1357,18 +1357,21 @@ def get_yaml(self, out): | |||
|
|||
class table: | |||
"""User provided thermo table for BinarySolutionTabulatedThermo""" | |||
def __init__(self, moleFraction=([],''), enthalpy=([],''), entropy=([],'')): | |||
def __init__(self, moleFraction=([],''), enthalpy=([],''), entropy=([],''), partialMolarVolume=([],'')): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the CTI/XML formats are going to be removed in the next few versions, we are preferring to add new features only to the YAML format. Unless there is already a significant userbase for this feature who are using CTI/XML files, we'd prefer to not change any of the converters.
else{ | ||
vbar.resize(x.size()); | ||
for(size_t i=0; i<N; i++){ | ||
vbar[i]=m_speciesMolarVolume[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please only use spaces for indentation, with 4 spaces per indent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dschmider-HSOG Depending on what code editor you are using, you can likely set this as a preference, so that a tab automatically inserts 4 spaces.
// Check for partialMolarVolume tag in tabulatedThermo table, take m_speciesMolarVolume[0] if false (for backward compatibility) | ||
if (table.hasKey("partialMolarVolume")){ | ||
vbar = table.convertVector("partialMolarVolume", "m3/kmol", N); | ||
for(size_t i=0; i<N; i++){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the style elsewhere, there should be spaces around the =
and <
. Also, one space before {
. This occurs in a few places.
@@ -90,26 +91,50 @@ void BinarySolutionTabulatedThermo::initThermo() | |||
size_t N = x.size(); | |||
vector_fp h = table.convertVector("enthalpy", "J/kmol", N); | |||
vector_fp s = table.convertVector("entropy", "J/kmol/K", N); | |||
vector_fp vbar; | |||
// Check for partialMolarVolume tag in tabulatedThermo table, take m_speciesMolarVolume[0] if false (for backward compatibility) | |||
if (table.hasKey("partialMolarVolume")){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (table.hasKey("partialMolarVolume")){ | |
if (table.hasKey("partialMolarVolume")) { |
if (table.hasKey("partialMolarVolume")){ | ||
vbar = table.convertVector("partialMolarVolume", "m3/kmol", N); | ||
for(size_t i=0; i<N; i++){ | ||
vbar[i]+=m_speciesMolarVolume[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure there is one space on each side of the +=
.
} | ||
} | ||
else{ | ||
vbar.resize(x.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use N
here?
@@ -161,29 +186,51 @@ void BinarySolutionTabulatedThermo::initThermoXML(XML_Node& phaseNode, const std | |||
getFloatArray(dataNode, h, true, "J/kmol", "enthalpy"); | |||
getFloatArray(dataNode, s, true, "J/kmol/K", "entropy"); | |||
|
|||
// Check for partialMolarVolume tag in tabulatedThermo table, take m_speciesMolarVolume[0] if false (for backward compatibility) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned earlier, unless there is a substantial existing userbase for this feature, there's no need to add it to the XML reader.
if (i==0 and moleFraction[0]==0){ | ||
integratedData[i]=0; | ||
} | ||
else if (i==0 and moleFraction[0]>0){ | ||
integratedData[i]=moleFraction[i]*inputData[0]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can avoid this conditional by doing the first element outside the loop and starting the loop at 1 instead of 0. Also, these two conditions are identical, since if moleFraction[i] == 0
is true, then moleFraction[i]*inputData[i]
will also be 0. There's no need to have the condition, I think you can just do the multiplication.
Hi, @dschmider-HSOG , I'll echo @bryanwweber's comments - many thanks for this excellent contribution! In addition to @bryanwweber's comments on the coding style, there are a few places where a little bit more documentation will help a user understand some of this functionality.
Other than that, I'll leave a few small comments here and there, and like Bryan, am happy to help get this across the finish line, as needed. It is really quite close to ready, though. Thanks again! |
|
||
void BinarySolutionTabulatedThermo::calcDensity() | ||
{ | ||
// Fallback to IdealSolidSolnPhase::calcDensity() if called before m_integrated_partial_molar_volume_tab is initialized |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand why you cannot use the other routine if m_integrated_partial_molar_volume_tab
is initialized, but wouldn't this still be an erroneous value?
Is there a specific case where this will be called before the table is initialized, during setup of the class, or is it possible that a user will call it before the initialization is complete? If the latter, I'd rather find a way for this call to trigger the initialization, or throw an informative error message, rather than returning an incorrect value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is sort of a workaround. When I implemented the calcDensity() function, the code crashed because the calcDensity() routine got invoked when m_integrated_partial_molar_volume_tab was still empty.
I wondered how this is possible because actually m_integrated_partial_molar_volume_tab is initialized at the end of void BinarySolutionTabulatedThermo::initThermo(), but I could not track down, which function called calcDensity() before initThermo() initialized the phase. So that was the only solution I could come up with.
Hi, |
HI @dschmider-HSOG Welcome back! It's up to you whether to amend the previous commit or add new ones. I usually try to keep a logical unit of work per commit. |
Hi, |
Hi @dschmider-HSOG Sure, editing that same file should be fine. Thanks! |
@dschmider-HSOG are you able to finish this pull request up? It looks like a neat function that'd be great to have merged. Please let me know if you have any questions! |
Hi @bryanwweber, thanks for asking. I was occupied with other topics in the meantime, but should soon be able to complete the commit. |
Enable to consider non-constant partial molar volumes provided by tabular data, since the partial molar volume of typical battery intercalation materials shows strong dependency on e.g. lithium stoichiometry. The tabular data is included in the same table as enthalpy and entropy data. -implement optional partialMolarVolume keyword in table(thermo) -BinarySolutionTabulatedThermo::interpolate is adjusted to be more general and also interpolate partial molar volume -BinarySolutionTabulatedThermo::integrate is added since partial molar volumes are often needed in integrated form (integrated over lithium stoichiometry) -BinarySolutionTabulatedThermo::calcDensity is adjusted since density is not constant anymore and changes with varying partial molar volumes
- Correct several formatting/style issues - Revert changes introduced in XML converters
- Change the implementation to provided molar volumes instead of partial molar volumes - Extend BinarySolutionTabulatedThermo_Test.cpp to test for added functionality - Add molarVolume data to BinarySolutionTabulatedThermo.yaml - Edit documentation in header file
8782ea6
to
1fc652f
Compare
Hi, |
Codecov Report
@@ Coverage Diff @@
## main #1072 +/- ##
==========================================
- Coverage 65.47% 65.47% -0.01%
==========================================
Files 327 327
Lines 46350 46416 +66
Branches 19688 19718 +30
==========================================
+ Hits 30349 30391 +42
- Misses 13476 13496 +20
- Partials 2525 2529 +4
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
//! Numerical derivation of the molar volume table | ||
/*! | ||
* Tabulated values are only interpolated within the limits of the provided mole | ||
* fraction. If these limits are exceeded, the values are capped at the lower or | ||
* the upper limit. | ||
*/ | ||
void derive(vector_fp& inputData, vector_fp& derivedData, | ||
vector_fp& moleFraction); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments on this function:
- This is the numerical derivative of the molar volume
- I think
diff
would be a better name for this function is used, the mole fraction vector ism_molefrac_tab
- I don't think the
moleFraction
argument is needed -- the only way this function is called, - The usual order of arguments would be to have the input arguments (
inputData
andmoleFraction
first, followed by the output argument. - The input arguments should be marked as
const
- The function itself is can be
const
as well.
//! Species thermodynamics linear interpolation function | ||
/*! | ||
* Tabulated values are only interpolated within the limits of the provided mole | ||
* fraction. If these limits are exceeded, the values are capped at the lower or | ||
* the upper limit. | ||
*/ | ||
double interpolate(double x, const vector_fp& molefrac, | ||
const vector_fp& inputData) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the molefrac
vector needs to be an input argument -- it is always the m_molefrac_tab
variable which is available in this function as well.
It would be useful to document the input and output arguments in the method docstring, e.g. with @param x (some description
and @returns the value of ...
.
include/cantera/thermo/ThermoPhase.h
Outdated
//@} | ||
/// @name Properties of the Standard State of the Species in the Solution | ||
//@{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change should be reverted - the lines containing @{
and @}
need to be Doxygen comment lines (starting with //!
or ///
, as they were.
@@ -43,6 +43,16 @@ phases: | |||
19.2971, 19.2977, 16.2213, 19.298, 19.2978, 19.2945, 19.2899, 19.2877, | |||
19.2882, 19.2882, 19.2882, 19.2882, 19.2882, 19.2885, 19.2876, 19.2837, | |||
19.2769, 19.285, 19.31, 19.3514] | |||
molarVolume: [34.582988, 34.677730, 34.776275, 34.875760, 34.973980, 35.069268, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the YAML format, we generally use lowercase words separated by spaces, so this should be molar-volume
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for providing these updates. I appreciated the detailed description of the additions in the class docstring.
Besides the handful of comments above and below*, I found an alternative to duplicating the implementation of addSpecies
from IdealSolidSolnPhase
, which I've pushed to my fork (https://github.com/speth/cantera/tree/tabulated_partial_molar_volumes). You can either cherry-pick that commit onto your branch here, or if you want I can push that change to this branch.
*sorry, something funny happened with the GitHub plugin for VS Code and it posted part of this before I was finished reviewing -- please take a look at the comments from the prior review as well.
{ | ||
{ | ||
double vmol = interpolate(Phase::moleFraction(m_kk_tab), m_molefrac_tab, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an extra, unnecessary layer of braces and indentation around the body of this function.
Also, I don't think the Phase::
qualifier is needed before calling moleFraction
(here or anywhere else in this class).
} | ||
} | ||
|
||
void BinarySolutionTabulatedThermo::getPartialMolarVolumes(doublereal* vbar) const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a rogue doublereal
here that should just be double
(likewise for this method in the header file).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw that in the generation of the doxygen file, the information that this function is reimplemented from the parent class is lost if I change to double. Therefore I thought it would be better to keep doublereal. But sure I can change this to double.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. I thought Doxygen was smart enough to see through the typedef
and know that these methods were the same. I'd suggest changing it to double
anyways. We'll get around to eliminating doublereal
on a wider scale eventually.
} | ||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A minor formatting suggestion:
} | |
else { | |
} else { |
Thanks @speth for reviewing. I've implemented your suggestions. Regarding |
That's true in most cases, but not if you add species to the phase after the fact (which is of course not allowed in the case of |
Hi @dschmider-HSOG, I think you haven't yet pushed the updates to this branch. |
I see. But wouldn't the implementation with the |
22c2045
to
3e3ab4d
Compare
Yes, that's a good point. But this can be a pretty minimal implementation, which just checks that condition an then calls |
- Edit method docstrings in header file - Change minor formatting issues
3e3ab4d
to
9fd67db
Compare
Sorry, I somehow overlooked the fact that
I changed my last commit to incorporate the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, all of these changes look good to me. One other small thing that I forgot to mention is that there should be an update to doc/sphinx/yaml/phases.rst
to describe the additional parameter in the YAML API documentation. This file is used to populate the information that ends up here on the Cantera website.
- Add description for molar-volume parameter to doc/sphinx/yaml/phases.rst in binary-solution-tabulated
Thank you @decaluwe, @bryanwweber and @speth for helping me finish my very first pull request. I really appreciated your constructive feedback. Best regards, |
Congrats, @dschmider-HSOG - so excited for your contribution! |
Enable to consider non-constant partial molar volumes provided by tabular data, since the partial molar volume of typical battery intercalation materials shows strong dependency on e.g. lithium stoichiometry. The tabular data is included in the same table as enthalpy and entropy data.
-implement optional partialMolarVolume keyword in table(thermo)
-BinarySolutionTabulatedThermo::interpolate is adjusted to be more general and also interpolate partial molar volume
-BinarySolutionTabulatedThermo::integrate is added since partial molar volumes are often needed in integrated form (integrated over lithium stoichiometry)
-BinarySolutionTabulatedThermo::calcDensity is adjusted since density is not constant anymore and changes with varying partial molar volumes
Checklist
scons build
&scons test
) and unit tests address code coverage