BinarySolutionTabulatedThermo Class#563
Conversation
|
Thanks for this PR! I'm glad to see this getting into shape. As a first step toward getting this PR merged, could you revert the merge commit and rebase this onto the current |
Codecov Report
@@ Coverage Diff @@
## master #563 +/- ##
==========================================
+ Coverage 68.42% 68.48% +0.05%
==========================================
Files 360 363 +3
Lines 39791 39961 +170
==========================================
+ Hits 27227 27367 +140
- Misses 12564 12594 +30
Continue to review full report at Codecov.
|
|
Sorry, I should perhaps not have said 'revert'. What I really mean is to reset your branch to the state before the merge commit, e.g. |
benjaminkee
force-pushed
the
master
branch
2 times, most recently
from
60e35e2 to
71c3c32
Compare
|
Ah ok, thanks for the git tips! I just pushed now with the instructions provided, with the exception of the reset commit, which was changed to one later commit, to include the test suite files. |
|
The Linux part of the travis check is failing due to |
|
Thanks for the help! Looks like all tests pass now. |
bryanwweber
left a comment
bryanwweber
left a comment
There was a problem hiding this comment.
Hello! 👋 Thank you again for making this pull request! I've gone through and made some stylistic comments about the code and the formatting. In general, things look pretty good, but there are some things to change to make your code consistent with the rest of Cantera. Take a look at our Contributing guide for more information, in addition to my comments here.
It would be super if you could add an example of how to use this new functionality, aside from just the tests - what motivated you to add these classes? What kind of problems are you solving?
speth
left a comment
speth
left a comment
There was a problem hiding this comment.
I have a few overarching questions/suggestions for this PR, as well as some more detail-oriented issues which will ultimately need to be addressed. I would suggest we address these questions first, before worrying about some of the implementation details.
- Is there a need for introducing two classes here? What does the
ConstDensityTabulatedThermoclass do that theIdealSolidSolnPhaseTabulatedThermoclass can't? - Can you provide a physical/mathematical description of how this model is meant to work? There are a few things that aren't quite clear when trying to work backward from the implementation.
- I would strongly prefer the input format for the new model to make use of the existing CTI/XML input file formats, rather than introducing a new, ad hoc format. It should also be possible to provide all necessary inputs by calling functions in the C++ code, rather than needing to use any files. I am happy to help with this if need be.
"Cantera#563" - Removed ConstDensityTabulatedThermo class - Modified IdealSolidSolnPhaseTabulatedThermo class - Modified ctml_writer.py
|
We've updated this pull request to reflect the changes brought up by @bryanwweber and @speth.
Please do let us know if any further changes are required. |
speth
left a comment
speth
left a comment
There was a problem hiding this comment.
Thank you for the updates. This looks to be significantly improved, however there are still a number of issues that should be addressed. I believe all of the comments made earlier that are not already marked as resolved are still applicable. Please ask if you have questions about any of them.
-
Please make sure to commit files without using Windows-style line endings. You can do this with the git setting:
git config --global core.autocrlf trueThen, to fix the existing affected files (
BinarySolutionTabulatedThermo.h,BinarySolutionTabulatedThermo.cpp, andlithium_ion_battery.cti), I think you should be able to do something likegit add --renormalize path/to/fileYou may need to update your version of Git (2.16 or newer), as this is a fairly new feature.
-
I very much appreciate the added Matlab example. One thing that would make this even more useful would be including a script which calls this function, e.g. doing a parameter sweep of some sort and plotting the results. Also, I'm assuming that it would eventually make sense to provide a similar example in Python, in which case I would suggest moving the
lithium_ion_battery.ctifile to thedata/inputsdirectory. -
I have a number of concerns about the the implementation of the
_updateThermomethod. I am not sure that it is doing what you intend in all cases, and I think there may be a simpler way to do some of these things, which is why I was hoping you could share a description of the model. -
Please remember to add yourselves to the
AUTHORSfile
benjaminkee
changed the title
|
Somehow, a number of commits that shouldn't have been part of this PR (related to Steven's other PR with the critical properties database) got included here. I removed those from this PR and squashed several commits to reduce the churn associated with addressing some of the comments on the PR, and rebased this on top of the current master. Please base all further work on this updated branch to make merging this PR easier. A more serious issue arose while I was trying to fix the problem of the shadowed member variables, specifically |
|
Okay, I think I have fixed up the tests. Main thing was that IdealSolidSolnPhase.h needed to be included in the test file header. After fixing this, there was still a small discrepancy between some of the test return values and the My latest push to this branch incorporates your fixes from your branch, and then adds my additional fixes. I think the only things left to do are:
@speth and @bryanwweber , does this list sum up the remaining changes, in your view? |
|
Sorry - not quite sure how I closed this PR, but it is now re-opened... |
| nt = len(self._transport) | ||
| for n in range(nt): | ||
| self._transport[n].build(t) | ||
| if self._standardState: |
There was a problem hiding this comment.
Just want to check that the behavior here is expected: this will be falsey if _standardState is None, '', 0, [], etc. If any of those should be allowed values, the check should instead be if self._standardState is not None:
There was a problem hiding this comment.
This is correct. The accepted inputs are all strings: unity, molar_volume, and solvent_volume. These are re-assigned to an int value, c.f. IdealSolidSolnPhase::setStandardConcentrationModel:
void IdealSolidSolnPhase::setStandardConcentrationModel(const std::string& model)
{
if (caseInsensitiveEquals(model, "unity")) {
m_formGC = 0;
} else if (caseInsensitiveEquals(model, "molar_volume")) {
m_formGC = 1;
} else if (caseInsensitiveEquals(model, "solvent_volume")) {
m_formGC = 2;
} else {
throw CanteraError("IdealSolidSolnPhase::setStandardConcentrationModel",
"Unknown standard concentration model '{}'", model);
}
}
So it should, in fact be "falsey" for any of those mentioned cases. 😄
Now, whether we want to enable someone to directly set the int value from input is a valid question, but imo is separate from this PR.
There was a problem hiding this comment.
I think users setting strings is much better than "magic numbers", so I vote against allowing to set the integers directly 😄
There was a problem hiding this comment.
I think this discussion is a bit confused -- the "standard state" that's being set here would be an object of type constantIncompressible, or nothing. The strings molar_volume etc. are for setting the standard_concentration property of the phase object.
There was a problem hiding this comment.
Oops - you're right. Okay, so can you comment on @bryanwweber 's query? It would look like it is used correctly.
If somebody enters something other than an instance of contantIncompressible, it looks like it repeats the build of the species thermo...?
There was a problem hiding this comment.
I agree, the else case for the _standardState makes no sense. I think it should be more like if self._standardState: build, otherwise do nothing.
There was a problem hiding this comment.
So skip the entire check to make sure the string assigned is actually an instance of the standardState class? Just based on how other classes operate in this file, I would think it would be:
if self._standardState:
ss = s.addChild("standardState")
ss['model'] = id
if isinstance(self._standardState, standardState):
self._standardState.build(ss)
and just omit the else statement. No?
speth
left a comment
speth
left a comment
There was a problem hiding this comment.
This looks like some good progress. Besides the issues noted below, there are a couple of items that I noted previously that I think should still be addressed:
- issue with units in calls to
getFloatArray - question about
_buildfunction ofstandardStateCTI class
| nt = len(self._transport) | ||
| for n in range(nt): | ||
| self._transport[n].build(t) | ||
| if self._standardState: |
There was a problem hiding this comment.
I agree, the else case for the _standardState makes no sense. I think it should be more like if self._standardState: build, otherwise do nothing.
|
Okay, unit conversion on |
decaluwe
force-pushed
the
master
branch
2 times, most recently
from
39110d5 to
56e29ba
Compare
Standard concentrations in the IdealMolalSolution phase depend on a user-specified m_formGC parameter, where m_formGC=0 results in a standard concentration of 1.0, m_formGC = 1 is supposed to result in a standard concentration for species k equal to 1 divided by the molar volume of species k, and m_formGC = 2 is supposed to result in a standard concentration equal to 1 divided by the molar volume of the solvent species (which is species 0). Current behavior is that m_formGC = 1 and m_formGC = 2 *both* result in a standard concentration of 1 divided by molar vlume of the solvent. This commit fixes how this is handled, cleans up the switch statement (the three cases were written somewhat inconsistently), and throws an error if m_formGC is set < 0 or > 2.
-Fixes small typo id incclude/cantera/base/utilities.h docstring -Removes `m_formGC` from BinarySolutionTabulatedThermo class, and instead utilizes version and functionality inherited from parent class `IdealSolidSolnPhase`. -Moves samples/matlab/lithium_ion_battery/lithium_ion_battery.cti to data/inputs/lithium_ion_battery.cti -Fixes typo in test/data/BinarySolutionTabulatedThermo.cti -Updates expected_result values in several test cases in test/thermo/BinarySolutionTabulatedThermo_Test.cpp
The keyword `standardState` was added to species::__init__ in ctml_writer.py. This moves this keyword entry to the end of the list of keywords, so that species instances of the class do not need to reorder their keyword order.
…ermo Previously the model imported the tabulated data assuming it was given in J, mol, K units, and ignoring any user input in the cti file, w/r/t units. This fixes that, by amending the `getFloatArray` calls in thermo/BinarySolutionTabulatedThermo.cpp
…hermo` class. -Removes option to read tabulated thermo from an external csv file (this is now handled from within cti or xml). -Renames `rateCoeff` keyword to the more appropriate `rate_coeff_type`, and fixing keyword order so that this new keyword is listed last. -Removes `else` statement from `if isinstance(self._standardState, standardState) -Removes unused `_pure` attribute from `IdealSolidSolution` and `BinarySolutionTabulatedThermo` -Changes default on `tabulated_species` keyword to `None`. -Removing superfluous `standardState:_build` from ctml_writer.py - Removes unnecessary conc_dim() definition in `table` class. - Removes unnecessary units defintion for mole fractions in `table` class. - Improves grammar in error message for case when thermo table is not provided for `tabulated_species`.
The function `BinarySolutionTabulatedThermo::interp` now returns type `std::pair<double, double>`, rather than `static double`
Previously, BinarySolutionTabulatedThermo::_updateThermo created a new `speciesThermoInterpType` intance every time the thermo was updated, storing the tabulated thermo lookups as the reference state thermo. This has now been changed such that the reference state is used only to represent the temperature effects on the thermo, with the tabulated terms added to this reference state. This should be a more efficient implementation.
Added some context and higher level functionality to lithium_ion_battery.m sample, such that it now uses some of the already-present functionality to calculate and plot the open circuit voltage for a lithium ion battery for a range of active material compositions.
The density of IdealSolidSolnPhase and BinarySolutionTabulatedThermo objects was not being computed as part of construction, causing code that interacted with them using setState/restoreState, such as the 'Solution' constructors in Matlab and Python, to fail.
The general intent here was to enable calculating reaction enthalpies in the Matlab toolbox, as part of the li-ion battery simulations in PR Cantera#563. This required several changes: - Create getDeltaEnthalpies.m in Matlab toolbox/@kinetics - Add kin_getDelta to kineticsmethods.cpp. Note that this can be easily extended to create other functions in the matlab Kinetics toolbox (see the other fucntions listed in kin_getDelta in src/clib/ct.cpp). - Add getPartialMolarEnthalpies to metalPhase class (it returns all zeros).
The general intent here was to enable calculating reaction enthalpies in the Matlab toolbox, as part of the li-ion battery simulations in PR Cantera#563. This required several changes: - Create getDeltaEnthalpies.m in Matlab toolbox/@kinetics - Add kin_getDelta to kineticsmethods.cpp. Note that this can be easily extended to create other functions in the matlab Kinetics toolbox (see the other fucntions listed in kin_getDelta in src/clib/ct.cpp). - Add getPartialMolarEnthalpies to metalPhase class (it returns all zeros).
The general intent here was to enable calculating reaction enthalpies in the Matlab toolbox, as part of the li-ion battery simulations in PR Cantera#563. This required several changes: - Create getDeltaEnthalpies.m in Matlab toolbox/@kinetics, as well as similar methods for Gibbs free energy and entropy of reaction - Add kin_getDelta to kineticsmethods.cpp. - Add getPartialMolarEnthalpies to metalPhase class (it returns all zeros). Note that similar methods are not enabled for the corresponding 'Standard State' methods, for the time being. Mainly because it is difficult for me to envision a significant use case, but also because of some lingering confusion between 'standard' and 'reference' states in Cantera's codebase.
The general intent here was to enable calculating reaction enthalpies in the Matlab toolbox, as part of the li-ion battery simulations in PR #563. This required several changes: - Create getDeltaEnthalpies.m in Matlab toolbox/@kinetics, as well as similar methods for Gibbs free energy and entropy of reaction - Add kin_getDelta to kineticsmethods.cpp. - Add getPartialMolarEnthalpies to metalPhase class (it returns all zeros). Note that similar methods are not enabled for the corresponding 'Standard State' methods, for the time being. Mainly because it is difficult for me to envision a significant use case, but also because of some lingering confusion between 'standard' and 'reference' states in Cantera's codebase.
The general intent here was to enable calculating reaction enthalpies in the Matlab toolbox, as part of the li-ion battery simulations in PR Cantera#563. This required several changes: - Create getDeltaEnthalpies.m in Matlab toolbox/@kinetics, as well as similar methods for Gibbs free energy and entropy of reaction - Add kin_getDelta to kineticsmethods.cpp. - Add getPartialMolarEnthalpies to metalPhase class (it returns all zeros). Note that similar methods are not enabled for the corresponding 'Standard State' methods, for the time being. Mainly because it is difficult for me to envision a significant use case, but also because of some lingering confusion between 'standard' and 'reference' states in Cantera's codebase.
The general intent here was to enable calculating reaction enthalpies in the Matlab toolbox, as part of the li-ion battery simulations in PR Cantera#563. This required several changes: - Create getDeltaEnthalpies.m in Matlab toolbox/@kinetics, as well as similar methods for Gibbs free energy and entropy of reaction - Add kin_getDelta to kineticsmethods.cpp. - Add getPartialMolarEnthalpies to metalPhase class (it returns all zeros). Note that similar methods are not enabled for the corresponding 'Standard State' methods, for the time being. Mainly because it is difficult for me to envision a significant use case, but also because of some lingering confusion between 'standard' and 'reference' states in Cantera's codebase.
5 participants
Changes proposed in this pull request: