SolutionArray/ flamebase importers allow unnormalized and/or negative mass/mole fractions

[DavidAkinpelu]

Changes proposed in this pull request

• SolutionArray/ Flambase importers allow negative values for mass and mole fractions
• SolutionArray appends Solution object with unnormalized mass/mole fractions

If applicable, fill in the issue number this pull request is fixing

Closes #1016

If applicable, provide an example illustrating new features this pull request is introducing

Operating system: Windows 10
Python Version: 3.9.2
Cantera version: 2.6.0a1

In [1]: import cantera as ct
   ...: gas = ct.Solution('h2o2.yaml')
   ...: gas.set_unnormalized_mole_fractions(gas.X - 1e-16)
   ...: arr = ct.SolutionArray(gas, 5)

In [2]: arr.X
Out[2]: 
array([[ 1.e+00, -1.e-16, -1.e-16, -1.e-16, -1.e-16, -1.e-16, -1.e-16,
        -1.e-16, -1.e-16, -1.e-16],
       [ 1.e+00, -1.e-16, -1.e-16, -1.e-16, -1.e-16, -1.e-16, -1.e-16,
        -1.e-16, -1.e-16, -1.e-16],
       [ 1.e+00, -1.e-16, -1.e-16, -1.e-16, -1.e-16, -1.e-16, -1.e-16,
        -1.e-16, -1.e-16, -1.e-16],
       [ 1.e+00, -1.e-16, -1.e-16, -1.e-16, -1.e-16, -1.e-16, -1.e-16,
        -1.e-16, -1.e-16, -1.e-16],
       [ 1.e+00, -1.e-16, -1.e-16, -1.e-16, -1.e-16, -1.e-16, -1.e-16,
        -1.e-16, -1.e-16, -1.e-16]])
In [3]: arr.write_hdf('test.h5') # save to HDF
Out[3]: 'group0'

In [4]: gas_new = ct.Solution('h2o2.yaml')
   ...: arr_new = ct.SolutionArray(gas_new)
In [5]: arr_new.read_hdf('test.h5') # load from HDF
   ...: arr_new.X
Out[5]: 
array([[ 1.e+00, -1.e-16, -1.e-16, -1.e-16, -1.e-16, -1.e-16, -1.e-16,
        -1.e-16, -1.e-16, -1.e-16],
       [ 1.e+00, -1.e-16, -1.e-16, -1.e-16, -1.e-16, -1.e-16, -1.e-16,
        -1.e-16, -1.e-16, -1.e-16],
       [ 1.e+00, -1.e-16, -1.e-16, -1.e-16, -1.e-16, -1.e-16, -1.e-16,
        -1.e-16, -1.e-16, -1.e-16],
       [ 1.e+00, -1.e-16, -1.e-16, -1.e-16, -1.e-16, -1.e-16, -1.e-16,
        -1.e-16, -1.e-16, -1.e-16],
       [ 1.e+00, -1.e-16, -1.e-16, -1.e-16, -1.e-16, -1.e-16, -1.e-16,
        -1.e-16, -1.e-16, -1.e-16]])

In[6]: gas1 = ct.Solution('h2o2.yaml')
   ...: states = ct.SolutionArray(gas1)    
   ...: states.append(T=300., P=ct.one_atm, X = gas.X -1.e-16, normalize = False)    
In[7]: states.X
Out[7]:
array([[ 1.e+00, -1.e-16, -1.e-16, -1.e-16, -1.e-16, -1.e-16, -1.e-16,
        -1.e-16, -1.e-16, -1.e-16]])

Checklist

• The pull request includes a clear description of this code change
• Commit messages have short titles and reference relevant issues
• Build passes (scons build & scons test) and unit tests address code coverage
• Style & formatting of contributed code follows contributing guidelines
• The pull request is ready for review

[ischoegl]

Thanks, @DavidAkinpelu. As I am involved with this, I’ll leave it to @speth and/or @bryanwweber to review.

[bryanwweber]

Hi @DavidAkinpelu! Thank you for contributing this change, and welcome to contributing to Cantera! I've made a few comments below. I think overall the change is a great addition and it'll certainly make the SolutionArray more useful. The comments are mainly aimed at simplifying the code and hopefully making it a little easier for the next person to understand!

[bryanwweber]

Thanks @DavidAkinpelu! These changes are looking great. I have a few more comments, mostly to clarify wording and text formatting in a few places.

[ischoegl]

Minor things.

[bryanwweber]

Thanks @DavidAkinpelu! This is very close, great work so far.

[bryanwweber]

Sorry, two more comments:

1. It'd be good to add a test for the normalize=True case of restore_data() (although I guess that may be covered by other tests?)
2. I'm still conflicted about the choice of normalize=False as the default value for restore_data, but especially for read_csv and read_hdf. The reasons I'm conflicted are: 1) it'd be the only setter (I think) that works this way by default, which increases the difficulty of using Cantera slightly; 2) it's a breaking change in behavior; but on the other hand 3) users who save data out from a SolutionArray would expect to restore exactly that data back if the use a function called restore_data. Maybe a compromise is to set the default to True for the tworead_* functions, and leave it False for restore_data? In my head, that resolves some of the conflict in terms of user expectations based on the function names, but then you get two different default values for very similar functions.

Since I really can't decide, I'm going to ping @speth for a third opinion.

[ischoegl]

Maybe a compromise is to set the default to True for the two read_* functions, and leave it False for restore_data?

I’d be ok with that compromise. For reading from file, not getting back what was saved was the starting point for this PR, and it certainly should not be normalized in that case by default. Normalize is only enforced by default when composition is explicitly set; otherwise Cantera allows for deviations.

[bryanwweber]

@DavidAkinpelu I'm going to reply in a top level comment here so that I can have some more space. I guess what you mean is this:

import cantera as ct
gas
In [2]: gas = ct.Solution("h2o2.yaml")
In [3]: import numpy as np
In [5]: x = np.ones(gas.n_species) * 0.3
In [6]: gas.set_unnormalized_mole_fractions(x)
In [9]: gas.X
Out[9]: array([0.3, 0.3, 0.3, 0.3, 0.3, 0.3, 0.3, 0.3, 0.3, 0.3])
In [10]: gas.Y
Out[10]:
array([0.00912106, 0.00456053, 0.07238482, 0.14476964, 0.07694535,
       0.08150588, 0.14933017, 0.1538907 , 0.18074715, 0.1267447 ])
In [11]: gas.mean_molecular_weight
Out[11]: 66.3081
In [13]: np.dot(gas.X, gas.molecular_weights)
Out[13]: 66.3081
In [16]: 1/np.dot(gas.Y, 1/gas.molecular_weights)
Out[16]: 22.102699999999995

If the system were in a consistent state, those would be the same values, but they're not. As far as I can see, this kind of transformation is inevitable since the conversion from mole to mass fraction relies on the sum of them being equal to 1.0.

In the end, I don't think the right procedure is to simply disable the non-normalized mole fraction setter when Y is in the natural states set (at least, I think that's happening here. I'm having a hard time following the code). One should not expect the mass fractions to also be non-normalized if non-normalized mole fractions are set, and vice versa, for the reason I stated above. IMO, the code you had here before was much simpler and more robust (other than the test case).

In addition, I think it would be a better user experience if the normalize and states keyword-arguments were mutually exclusive in append(). I don't see any cases for using both together, and if it's needed, simply getting TPX and appending that would work just as well.

[DavidAkinpelu]

Thanks for the clarification, you couldn't have explained it better. You are right. in the previous case, appending using TPX failed, the tests were only able to pass through because 1.e-16 is within the tolerance.

[ischoegl]

Linking #390 as this is related.

[speth]

Allowing users the option to preserve mass fraction arrays that do not sum to 1 and contain negative values makes sense to me, but I think we should be careful about when that makes sense as a default behavior. I view working with such arrays as something that only a fairly experienced user should be trying to do, given the numerous pitfalls, and it's not unreasonable to expect such users to be able to set this option when they need it.

The case where I can see doing this by default is when the source of the mass fractions is a ThermoPhase object, where the only way that such a mass fraction vector could have been set in the first place is with one of the noNorm setters.

The case of restoring data from a file is a bit fuzzier. If you assume that the data came from Cantera, it could make sense, but I don't think that's an assumption that's universally true, and I think from the viewpoint of seeing this as an advanced option, I would recommend defaulting to the current behavior of normalizing the mass fractions.

When setting the state from a list or Numpy array, deciding not to normalize by default would be a significant departure from the behavior for the Solution object and the underlying Phase class, and I don't think the SolutionArray class should be a special case here.

[DavidAkinpelu]

@bryanwweber, @speth... The PR is updated and is ready for further review.

[speth]

Thanks, @DavidAkinpelu. This largely looks good to me. With the exception of the one case where mass and mole fractions are mixed up, most of my comments are just a few points where the code might be simplified a bit and some suggestions for the documentation.

@speth is handling final approval

[DavidAkinpelu]

@speth... The PR is updated and is ready for further review.

[speth]

Thanks for the updates, @DavidAkinpelu. I just saw one last typo that can be fixed. Otherwise, I think this is good to go.

[DavidAkinpelu]

@speth.. The PR has been updated.

[speth]

Looks good. Thank you!