Differentiate quality from mole fraction in Python

[ischoegl]

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

Fixes: N/A; but follows discussion in review of #687:

[...] it may make sense to use Q for quality at some point in the future. [...]

This would also be consistent with other naming conventions that have been adopted in the Python module, e.g. D for density instead of R for Rho. [...]

Changes proposed in this pull request:

• Use Q instead of X for quality (vapor fraction)
• Deprecate all instances using X in PureFluid
• In SolutionArray.collect, the species name is appended in labels, i.e. Q_H2O

I.e.

In [1]: import cantera as ct

In [2]: w = ct.Water()

In [3]: w.TX = 400, .5
DeprecationWarning: To be removed after Cantera 2.5. Attribute renamed to 'TQ'

In [4]: w.TQ
Out[4]: (400.0, 0.5)

[speth]

I'm fully on board with this renaming. I think it will be worth it to avoid the confusion caused by the overloading of X as both mole fraction and vapor fraction depending on the phase type.

[speth]

While trying to experiment with getting Q to work as the column name for SolutionArray output, I ran into a problem, and haven't quite been able to figure out why it's failing:

>>> import cantera as ct
>>> p = ct.Water()
>>> s = ct.SolutionArray(p, 5)
>>> data, cols = s.collect_data()
>>> s.restore_data(data, cols)

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-5-02284ff2df78> in <module>()
----> 1 s.restore_data(data, cols)

~/src/cantera/build/python/cantera/composite.py in restore_data(self, data, labels)
    704             raise ValueError(
    705                 "invalid/incomplete state information (detected "
--> 706                 "partial information as mode='{}')".format(mode)
    707             )
    708

ValueError: invalid/incomplete state information (detected partial information as mode='Y')

[ischoegl]

Hm, I can reproduce this. I will have to look at this, as it looks like the standard TD state (which is the default) is causing trouble. (Also: likely a pre-existing condition unrelated to the switch to Q.)

In [1]: import cantera as ct
   ...: p = ct.Water()
   ...: s = ct.SolutionArray(p, 5)
   ...: data, cols = s.collect_data()

In[2]: cols
Out[2]: ['T', 'density', 'Y_H2O']

Will add some additional unit tests to make sure that there aren't any surprises.

[ischoegl]

@speth ... on second look, the fix is already included in #720, which allows for different standard states (up until now, the standard state is defined as TDY regardless of the phase model; PR #720 sets it to TD for PureFluid).

So, would you mind tabling this until #720 is discussed & merged? (I recall you mentioning that there may be several issues that may need to be converged).

[speth]

Ah, I didn't realize that it was a pre-existing issue, but that makes sense, and I'm happy to see that it's already fixed by #720.

[ischoegl]

@speth ... I confirmed that the bug you found was indeed pre-existing, but also found a second issue that prevented #720 from fixing it directly (the additional fix is now added in ce424c9). This is added to #720, as a fix of #644 (and associated issues) is required regardless. I also added a unit test to confirm that things are working as expected.

[ischoegl]

Adding an observation from #720 here (per @speth's findings)

gas = ct.Solution('gri30.yaml')
w = ct.Water()
a = ct.SolutionArray(gas, 10)
'TX' in dir(a)  # False, as expected
b = ct.SolutionArray(w)
'TX' in dir(b)  # True, as expected
'TX' in dir(a)  # True, as expected, but not desired
a.TX  # unexpectedly (but as desired) raises AttributeError

Once TX and TQ are differentiated, all should have the expected/desired outcome.

[codecov]

Codecov Report

Merging #719 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #719      +/-   ##
=========================================
+ Coverage    71.4%   71.4%   +<.01%     
=========================================
  Files         372     372              
  Lines       43859   43863       +4     
=========================================
+ Hits        31317   31320       +3     
- Misses      12542   12543       +1

Impacted Files	Coverage Δ
src/thermo/PureFluidPhase.cpp	59.11% <100%> (ø)	⬆️
include/cantera/thermo/Phase.h	86.04% <100%> (+0.68%)	⬆️
include/cantera/thermo/PureFluidPhase.h	84.61% <100%> (+2.79%)	⬆️
src/thermo/HMWSoln.cpp	71.54% <0%> (-0.05%)	⬇️

---

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 c4c30e3...211cb09. Read the comment docs.

[ischoegl]

Remaining steps:

• rebase to incorporate new features introduced in Make the concept of saving state vectors more general #720
• ensure that data, cols = s.collect_data() yields ['T', 'density']
• enforce single-letter property acronyms by changing signature of Phase::nativeState from std::map<std::string, size_t> to std::map<char, size_t>
• confirm that SolutionArray has correct setters (i.e. includes TPQ etc.)
• introduce Phase::isFluid flag to facilitate identification of substances with phase change

[ischoegl]

@speth after rebasing to #720, SolutionArray appears to be working as expected.

In [1]: import cantera as ct
   ...: gas = ct.Solution('gri30.yaml')
   ...: w = ct.Water()
   ...: a = ct.SolutionArray(gas, 10)
   ...: b = ct.SolutionArray(w, 2)
   ...: 

In [2]: [st in dir(w) for st in ['TX', 'TQ']] # expected
Out[2]: [True, True]

In [3]: [st in dir(a) for st in ['TX', 'TQ']] # expected
Out[3]: [False, False]

In [4]: [st in dir(b) for st in ['TX', 'TQ']] # unexpected? <<<<<<<<<<<<<<<<<
Out[4]: [False, False]

In [5]: b.TQ # expected
Out[5]: (array([ 300.,  300.]), array([ 0.,  0.]))

In [6]: b.TQ = 400, .5

In [7]: b.TQ
Out[7]: (array([ 400.,  400.]), array([ 0.5,  0.5]))

I.e. I believe this is ready for a review.

[speth]

Currently, this eliminates the the TX and PX attributes of SolutionArray without a (runtime) deprecation warning. I also noticed that the X and TPX properties of SolutionArray don't seem to emit any warnings for pure fluids.

I'm not really sold on making nativeState work with char instead of std::string. The efficiency benefit shouldn't ever matter, and I don't want to assume that there will always be a clear one-character name for any state variable. For example, what letter would you use for the electron temperature in a non-thermal plasma?

The idea of having Q be dimensioned as nSpecies to support more general multi-phase fluids is an interesting one, but it's a bit inconsistent here -- the value returned by PureFluid.Q is a scalar, while SolutionArray.Q returns an array with nSpecies in the last dimension, and the second value returned by SolutionArray.TQ is not. I realize that this is an inconsistency inherited from the previous mis-use of X, but that's not a great reason to keep it. I'd like to avoid making the interface to the simple case that we already have (i.e. single-component fluids) less convenient in support of a capability that is mostly theoretical at present (would Q even be a useful state setter in such a case?).

Regarding isFluid, I'm not really sure I see what the use case is, but if we really want such a flag, then the name is going to need some work. isMultiphase would be one option, but I'd worry about distinguishing between "this model includes phase changes" and "the current state involves multiple phases".

[ischoegl]

@speth ... thanks for the review. I'm responding to your points before addressing major issue.

this eliminates the the TX and PX attributes of SolutionArray without a (runtime) deprecation warning. I also noticed that the X and TPX properties of SolutionArray don't seem to emit any warnings for pure fluids.

Yes, I just confirmed this. I hadn't anticipating this as I thought that the unit tests would catch - turns out that I was only testing deprecation of PureFluid. Need to pick over 750a80b to see what's going on.

I'm not really sold on making nativeState work with char instead of std::string.

I dropped the commit.

The idea of having Q be dimensioned as nSpecies to support more general multi-phase fluids is an interesting one, but it's a bit inconsistent here [...] I'd like to avoid making the interface to the simple case that we already have (i.e. single-component fluids) less convenient [...]

I want to believe that Cantera should be able to handle realistic multi-fuel vaporization applications eventually. My thought here was to exactly replicate the behavior of mass/mole fractions. I don't see much of an inconvenience as the only instance where Q_H2O etc. is visible is in HDF/CSV output. (It'd still be easy to suppress it for a pure substance, while keeping it as n_species. Since X and Y are gone for pure species input/output, there isn't any inconsistency.)

Regarding isFluid, I'm not really sure I see what the use case is, but if we really want such a flag, then the name is going to need some work.

I had PureFluid in mind, which would have both isPure and isFluid set to true. I do, however, see your point, and would just name it hasPhaseTransition. The reason why I'd like to see this flag being added is that for various equations of state, it would be immediately clear whether phase transitions are implemented or not. Some real gas EOS are capable of predicting the regions, but I wouldn't assume that all cases are covered.

[speth]

I don't see much of an inconvenience as the only instance where Q_H2O etc. is visible is in HDF/CSV output.

I don't think Q_H2O showing up in the CSV/HDF5 output is particularly problematic. What I'm more concerned about is what is returned by Solution.Q when the underlying phase is, say, either pure-fluid or Redlich-Kwong. Currently, this is a float for the former, while the latter is not implemented even at the C++ level, but if were, would presumably need to return an array of length n_species, But I don't think we want the return type to vary with the number of species, and I'd rather not have to write water.Q[0] to get the vapor fraction.

I don't think the vapor fraction of each species in a multiphase mixture can be used as part of a state setter in the same way as it can for a single-species phase, since it would over-specify the system per the Gibbs' Phase Rule, so I see this as more of a calculated property than part of the state. It could make sense to have two separate properties, one for the total vapor fraction across all species and one for the fraction of each species in the vapor phase. This would be analogous to what we do for many other properties, e.g. partial_molar_enthalpies versus enthalpy_mass, where the latter is available as part of state setters like HP.

The reason why I'd like to see this flag being added is that for various equations of state, it would be immediately clear whether phase transitions are implemented or not

OK, I can see that being useful for cases like WaterSSTP where presumably the correct return value is false.

[ischoegl]

I don't think the vapor fraction of each species in a multiphase mixture can be used as part of a state setter in the same way as it can for a single-species phase, since it would over-specify the system per the Gibbs' Phase Rule, so I see this as more of a calculated property than part of the state.

This is a valid point: a scalar it is.

... WaterSSTP, Redlich-Kwong, etc.

Exactly - it's often not immediately clear what's implemented. I hope we'll be able to set hasPhaseTransition to true for more than just PureFluid eventually (same for the pending Peng-Robinson PR).

[ischoegl]

@speth ... per discussion, I switched isFluid to hasPhaseTransition and made Q a scalar.

The remaining issue is that it appears that setters/getters defined by

getter, setter = _state2_prop(name, Solution)

don't overload in derived classes? Will get back to it next week.

[ischoegl]

@speth ... I believe 174c9b7 addresses setters/getters not being defined correctly, i.e. almost all points previously raised are now addressed.

The lack of warnings is curious: I verified that the functions that should raise the warnings are indeed called. I found another context where warnings (and even errors!) are not raised, which may be related: see #746

[bryanwweber]

@ischoegl For warnings at least, the warning is only shown the first time a method is passed... i.e., the signature of the function is warn(method, message), where the method is stored and the message is only shown the first time that method is encountered. This is done in C++ so applies to all the interfaces. Not sure if that's the issue you're encountering (and certainly, this is not the resolution to #746).

[ischoegl]

For warnings at least, the warning is only shown the first time a method is passed...

Correct. The warnings here are, however, raised in pyx files, so the issue cannot be caused by the C++ implementation.

[ischoegl]

@bryanwweber and @speth ... documenting the lack of warning (which I believe to be a bug that goes beyond this PR)

In [1]: import cantera as ct
   ...: gas = ct.Solution('gri30.yaml')
   ...: w = ct.Water()
   ...: w.TQ = 300., .5
   ...: a = ct.SolutionArray(gas, 10)
   ...: b = ct.SolutionArray(w, 2)
   ...: 

In [2]: b.TPX # <--- no warning issued
Out[2]: 
(array([ 300.,  300.]), array([ 3528.21380229,  3528.21380229]), array([[ 0.5],
        [ 0.5]]))

In [3]: w.TPX # <--- calling the method raising the warning directly
/usr/bin/ipython3:1: DeprecationWarning: To be removed after Cantera 2.5. Attribute renamed to 'TPQ'
  #! /bin/sh
Out[3]: (300.0, 3528.213802290444, 0.5000000000000002)

In [4]: b.TPQ
Out[4]: 
(array([ 300.,  300.]),
 array([ 3528.21380229,  3528.21380229]),
 array([ 0.5,  0.5]))

[speth]

Thanks for the updates. This is looking pretty good to me.

I think there are some examples that need to be updated to use these new properties. rankine.py comes to mind immediately, but there may be others.

[ischoegl]

@speth ... made all requested changes and updated rankine.py (couldn't find other examples that are affected)