Make the concept of saving state vectors more general

[ischoegl]

Please fill in the issue number this pull request is fixing:

Fixes #595, fixes #644,

Changes proposed in this pull request:

• Define valid state combinations in C++ layer
• Ensure all phases covered by thermo-models.yaml can be saved and restored correctly
• Fix is needed for Python wrapper classes for NASA9 and piecewise Gibbs #711 and Add script to convert CTML to YAML input format #693
• Make SolutionArray (almost) phase agnostic

E.g. using the examples from #595 and #644, the behavior is now correct, and invalid setters are disabled (or deprecated).

In [1]: import cantera as ct
   ...: phase = ct.ThermoPhase('test_problems/cathermo/issp/IdealSolidSolnPhaseExample.xml')
   ...: T0, p0 = phase.TP # T0 = 500, p0 == 101325
   ...: z = phase.state # calls Phase::saveState
   ...: phase.TP = 600, 2*ct.one_atm
   ...: phase.state = z # calls Phase::restoreState
   ...: assert phase.T == T0, 'temperature'
   ...: assert phase.P == p0, 'pressure'
   ...: 

In [2]: phase.TP
Out[2]: (500.0, 101325.0)

In [3]: phase.TD = 500, 12
DeprecationWarning: IdealSolidSolnPhase::setDensity: Function does not change density and will be removed after Cantera 2.5.
[...]
CanteraError: 
***********************************************************************
CanteraError thrown by IdealSolidSolnPhase::setDensity:
Density is not an independent variable
***********************************************************************

In [4]: phase._native_state
Out[4]: ('T', 'P', 'Y')

In [5]: ysz = ct.Solution('sofc.cti','oxide_bulk')

In [6]: ysz.TD = 1073, 200
[...]
CanteraError: 
***********************************************************************
CanteraError thrown by Phase::assertCompressible:
Setter 'setDensity' is not available. Density is not an independent 
variable for 'oxide_bulk' ('Lattice')
***********************************************************************

In [7]: ysz._default_state
Out[7]: ('T', 'P', 'X')

[ischoegl]

@bryanwweber ... at this point, Phase::saveState and Phase::restoreState work for all thermo models covered in thermo-models.yaml, i.e. this PR is ready for review (I believe this fix addresses all potential issues connected to #595 you may run into for #693).

Note: other phases not covered by this PR can be very easily added by overloading Phase::isIncompressible(), Phase::isStoichPhase() and Phase::defaultState(). For the latter, I assumed that mass base (Y) is the default for compositions, with exception of lattice-related phases, which use mole base (X).

[ischoegl]

Left comments to avoid another force-push (all are taken care of now).

[speth]

Conditioning saveState and restoreState on isIncompressible makes sense to me, and is a fine resolution for #595. Strictly speaking, we would probably like to have saveState use the variables in defaultState and get the kind of behavior that this PR gets in SolutionArray.colect_data, but that has the potential to get messy, and I don't think we have any phase types right now where that would really be worthwhile.

Disabling the composition setters for stoichiometric phases seems unnecessary. The mass/mole fractions get normalized anyway, so any value specified there will have no effect in any case.

I'm not sure that it's worth the extra complexity of keeping track of which phases have fixed stoichiometry in order to reduce the size of the state used by Phase::saveState and Phase::restoreState by one variable. As it is, the the Python state property doesn't even handle this -- we would need to add yet another method which returned the size of the state depending on the value of isStoichPhase.

I think the best way to disable setting density to fix #644 and other equivalent scenarios is to override the setDensity method of the appropriate class at the C++ level. This achieves the desired behavior in all language interfaces, not just Python, and avoids the overhead of the _check_setter method.

After that, the only case in which the distinction between the "partial states" and "full states" is made is within SolutionArray, where it's still part of a special case, so I'm hoping there's an opportunity to simplify this somewhat.

[ischoegl]

@speth ... thanks for the comments!

Strictly speaking, we would probably like to have saveState use the variables in defaultState and get the kind of behavior that this PR gets in SolutionArray.collect_data, but that has the potential to get messy [...]

That is exactly what this PR is trying to accomplish 😁 (it's actually less messy than I thought at first)

I'm not sure that it's worth the extra complexity of keeping track of which phases have fixed stoichiometry in order to reduce the size of the state used by Phase::saveState and Phase::restoreState by one variable.

That's a minor side-effect of the implementation. I mainly needed to peel off the compositions for stoichiometric phases as this makes things consistent with the fullStates definitions (which are essential for SolutionArray.restore_data).

Your comment still pointed out one case I hadn't thought off (someone could add Y to the output columns of a stoichiometric substance and the import would fail, see PureFluid). I'll fix that one and likely add more unit tests for additional phases. Nevertheless, the solution as it stands at this point should already work for most thermo phases (also: this is where the availability of setters is important).

[ischoegl]

@speth ... I confirmed that this PR indeed allows SolutionArray objects being restored for any of the phases defined in thermo_models.yaml based on their defaultState definitions (only exception: 'ions-from-neutral-molecule'). Also, the remaining issue with PureFluid is fixed.

I added more detailed unit tests to that effect, i.e.

cantera/interfaces/cython/cantera/test/test_models.py

Line 56 in 16c5073

def test_restore_thermo_models(self):

[ischoegl]

Edit: the behavior described below is easy to fix (see f8abd72 pushed below)

There's one behavior I observed and am not sure about. For StoichSubstance, the setter for TPX does not make sense from the perspective of recovering the state from a minimal set of states (X would always be set to 1 regardless).

The current behavior for StoichSubstance is that the setter for TPX is disabled (i.e. it raises an AttributeError). For SolutionArray, I followed the past practice of only adding setters that are defined for phases (via _full_states etc.). As stoichiometric substances have a reduced set of valid state definitions, the derived SolutionArray thus does not define all setters that are defined for Solution. In the case of TPX, it becomes assignable (!) as a newly created class variable.

This potentially confusing (and pre-exisiting) behavior could be avoided by adding all of the setters that are defined for Solution regardless, and not just the ones that are defined by _full_states.

[ischoegl]

I think the best way to disable setting density to fix #644 and other equivalent scenarios is to override the setDensity method of the appropriate class at the C++ level.

I can look into that but with #652 in mind it's hard to keep track. As most of the setters are presumably still inherited from Phase/ThermoPhase, a C++ solution would still involve the equivalent of a check_setter method.

[speth]

OK, I think we're making progress here!

I like the name assignDensity -- it clearly sets this apart from setDensity.

I did notice a few added blank lines containing whitespace. If you could clean those up it would be appreciated. They are unfortunately not visible in the GitHub UI, but you can see them pretty clearly in the output of git diff if you have color formatting enabled.

[bryanwweber]

FYI, I just noticed in the documentation for ThermoPhase (since you've moved pressure() to Phase):

The distinction is that the methods
declared in ThermoPhase require knowing the particular equation of state of
the phase of interest, while those of class Phase do not, since they only
involve data values stored within the object.

[ischoegl]

@bryanwweber

FYI, I just noticed in the documentation for ThermoPhase (since you've moved pressure() to `Phase):

The distinction is that the methods
declared in ThermoPhase require knowing the particular equation of state of
the phase of interest, while those of class Phase do not, since they only
involve data values stored within the object.

Thanks for pointing this out. Will address in a final revision.

@speth ... I believe this is now converging.

This time around I decided to shorten density to D in Phase::nativeState (perceived this as a wart: everything else is a single letter, see new documentation of Phase::fullStates). This required a small change in SolutionArray.collect_data, where the D is turned into density.

PS: the signature of Phase::nativeState could be changed to std::map<char, size_t>, but I'll wait for feedback first.

[ischoegl]

FYI, I just noticed in the documentation for ThermoPhase (since you've moved pressure() to Phase):

The distinction is that the methods
declared in ThermoPhase require knowing the particular equation of state of
the phase of interest, while those of class Phase do not, since they only
involve data values stored within the object.

@bryanwweber ... I figure the differentiation between Phase and ThermoPhase in the description made sense when density was the exclusive property used to define a state. This won't work for incompressible substances, where pressure has to be saved and density has to be calculated based on an equation of state.

What would you suggest as a replacement for this description? Or would you suggest to move pressure back, let Phase::saveState throw a NotImplementedError, and add actual code in ThermoPhase::saveState? (same for restoreState). Curiously in that case Phase::setDensity does not make sense either so I'd suggest to just change the decription?

[bryanwweber]

I think it probably makes most sense to change the description, since as you note, not every phase has density as the independent variable.

[speth]

I wouldn't sweat over the distinction between Phase and ThermoPhase, and I think the adjustments to which methods are where that has been made here is fine. Since I can't really imagine a case where you would have a Phase that isn't a ThermoPhase, we could theoretically merge the two classes. I've avoided doing this partly because the two header files are large enough as it is.

[ischoegl]

@bryanwweber and @speth ... there were several instances where updates from this PR affect descriptions of Phase definitions. To facilitate review, I kept updates in a single commit (a582743).

[ischoegl]

@speth ... I believe that the SolutionArray setters are the only remaining issue. For a suggested resolution, I applied your suggested approach from 2550e48 verbatim, which I honestly believe is a very good solution.

[speth]

Regarding the SolutionArray state properties, I pushed an update to my branch (08cabf5) which achieves what I was after with at least the properties that are defined on ThermoPhase maintaining the existing behavior, and then any other properties being added dynamically. You should be able to add that to your branch without affecting the authorship information using git cherry-pick.

Thanks for making the updates to the documentation. My remaining comments just pertain to those updates.

[ischoegl]

You should be able to add that to your branch without affecting the authorship information using git cherry-pick

I tried that before, unsuccessfully. Are you writing to my repo or upstream? (git pull is up to date and git fetch upstream didn't yield the desired result. I assume I'm overlooking something?)

[speth]

You should be able to add that to your branch without affecting the authorship information using git cherry-pick

I tried that before, unsuccessfully. Are you writing to my repo or upstream? (git pull is up to date and git fetch upstream didn't yield the desired result. I assume I'm overlooking something?)

Neither, I'm pushing to my fork.

[ischoegl]

Neither, I'm pushing to my fork.

Pretty obvious (in hindsight! you even mentioned as much). Thus far, I haven't had your fork set up as a remote. Thanks for resolving!

[bryanwweber]

I'm deferring final review approval to @speth at this point. I think we've converged to a good place.

[ischoegl]

I'm deferring final review approval to @speth at this point. I think we've converged to a good place.

Thanks, Bryan!

[bryanwweber]

Thank you @ischoegl!

[ischoegl]

Thanks for the thorough review and discussion @speth!