BinarySolutionTabulatedThermo Class

[benjaminkee]

Changes proposed in this pull request:

• Thermo class ConstDensityTabulatedThermo
• Thermo class IdealSolidSolnTabulatedThermo
• Test files for both thermo classes

[speth]

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 master instead?

[codecov]

Codecov Report

Merging #563 into master will increase coverage by 0.05%.
The diff coverage is 79.45%.

@@            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

Impacted Files	Coverage Δ
include/cantera/base/utilities.h	85.71% <ø> (ø)	⬆️
src/thermo/IdealMolalSoln.cpp	41.86% <0%> (+0.28%)	⬆️
test/thermo/BinarySolutionTabulatedThermo_Test.cpp	100% <100%> (ø)
src/thermo/IdealSolidSolnPhase.cpp	58.76% <100%> (+1.62%)	⬆️
src/thermo/ThermoFactory.cpp	78.53% <100%> (+0.1%)	⬆️
...ude/cantera/thermo/BinarySolutionTabulatedThermo.h	33.33% <33.33%> (ø)
src/thermo/BinarySolutionTabulatedThermo.cpp	70.64% <70.64%> (ø)
include/cantera/thermo/IdealSolidSolnPhase.h	41.66% <0%> (-23.34%)	⬇️
include/cantera/thermo/ThermoPhase.h	27.04% <0%> (-0.82%)	⬇️
... and 3 more

---

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 d04fd8c...46c4877. Read the comment docs.

[speth]

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. git reset --hard 2a0838e. Then you should be able to run git rebase upstream/master (replacing upstream with whatever you have named the Cantera/cantera remote) and then do a force push.

[benjaminkee]

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.

[benjaminkee]

The Linux part of the travis check is failing due to
"Exception occurred:
File "/home/travis/.local/lib/python3.4/site-packages/docutils/nodes.py", line 567, in getitem
return self.attributes[key]
KeyError: 'latex'"
Any suggestion on how to troubleshoot this?

[bryanwweber]

@LIONkey This is due to an error in a dependency that has been fixed if you rebase your branch onto the current tip of the master branch. The command @speth gave you in his last comment should do it.

[benjaminkee]

Thanks for the help! Looks like all tests pass now.

[bryanwweber]

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]

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 ConstDensityTabulatedThermo class do that the IdealSolidSolnPhaseTabulatedThermo class 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.

[manikmayur]

We've updated this pull request to reflect the changes brought up by @bryanwweber and @speth.

• The scientific relevance of the new thermodynamic class is to model lithium-ion battery intercalation materials. The need for this class is to be able to use experimentally-obtained half-cell data as function of lithium mole fraction as tabulated input for standard-state thermodynamics. We (Mayur, DeCaluwe, Kee, Kee, Bessler) are currently preparing a journal manuscript demonstrating the capabilities of the new class.
• The tabulated thermo class has been condensed into a single class called BinarySolutionTabulatedThermo.
• We've cleaned up the formatting issues.
• The data input was modified to accept the inline format (CTI level and XML level) instead of another file. Optionally at the CTI level, an CSV file can still be read; but this option is not used in the test suite.
• The tabulated species is now referred to as the tabulated species instead of the modifiable species.

Please do let us know if any further changes are required.

[speth]

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 true
  

  Then, to fix the existing affected files (BinarySolutionTabulatedThermo.h, BinarySolutionTabulatedThermo.cpp, and lithium_ion_battery.cti), I think you should be able to do something like

  git add --renormalize path/to/file
  

  You 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.cti file to the data/inputs directory.

• I have a number of concerns about the the implementation of the _updateThermo method. 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 AUTHORS file

[speth]

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 m_formGC. As written here, the setting in the input is used to set the variable BinarySolutionTabulatedThermo::m_formGC, which, since it is declared in both header files, is different from the variable IdealSolidSolnPhase::m_formGC. However, it is the only latter variable that is actually used in the methods that are inherited from the IdealSolidSolnPhase class. When I fixed this (see https://github.com/speth/cantera/tree/tabulated-thermo), all of the tests for the class started to fail. What seems to be happening is that when using the molar_volume setting for the standard concentration, all the thermo properties end up being inf. I haven't had a chance to look any deeper into the cause.

[decaluwe]

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 expected_result values. I did the hand calculations and confirmed that the actual class behavior was correct, and overwrote the expected_result values.

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:

1. Remove residual csv functionality from ctml_writer.py, which is no longer used.
2. Perhaps modify the example file in manner you mention, to show some form of parametric variation (i.e. for example, plot open circuit voltage as a function of composition, or perhaps run a charge-discharge curve).
3. Perhaps construct a similar example file in Python or Jupyter.

@speth and @bryanwweber , does this list sum up the remaining changes, in your view?

[decaluwe]

Sorry - not quite sure how I closed this PR, but it is now re-opened...

[bryanwweber]

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:

[decaluwe]

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.

[bryanwweber]

I think users setting strings is much better than "magic numbers", so I vote against allowing to set the integers directly 😄

[speth]

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.

[decaluwe]

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...?

[speth]

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.

[decaluwe]

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]

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 _build function of standardState CTI class

[speth]

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.

[decaluwe]

Okay, unit conversion on getFloatArray calls should be fixed, now.