Cantera Units

[hallaali]

Changes proposed in this pull request

• Include a units package to enable the use of alternative units in Cantera (besides SI)
• Add template file to SConscript to autogenerate code
• Allows users to attach units to the input quantities when setting a state and Cantera to output quantities with units attached
• e.g. To set a state with temperature and pressure with units:
  gas.TP = Q_(80, "degF"), Q_(1, "atm")

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

Related to Cantera/enhancements#3

Checklist

• There is a clear use-case for this code change
• The commit message has a short title & references relevant issues
• Build passes (scons build & scons test) and unit tests address code coverage
• The pull request is ready for review

[bryanwweber]

Hi @hallaali This is fantastic! I've made a few comments for you to review below. Feel free to ask questions about the comments here.

As a follow up to our discussion today, the next step is to add some tests. You'll need to create a file called test_units.py in the tests subfolder here. At the top of that file do

import cantera.units as ct
from . import utilities

then each test is put as a method on a class. So you would do something like

class TestSolutionUnits(utilities.CanteraTest):
    def setUp(self):
        self.phase = ct.Solution("h2o2.yaml")

    def test_mass_basis(self):
        self.assertEqual(self.phase.basis_units, 'kg')
        self.assertQuantityNear(self.phase.density_mass, self.phase.density)

    def test_dimensions(self):
        dims = self.phase.T.dimensionality
        self.assertIn("[temperature]", dims)
        self.assertEqual(dims["[temperature]"], 1.0)

Check out the test_thermo.py file, and specifically the methods test_mass_basis, test_molar_basis, check_setters, test_setState_mass, test_setState_mole, test_setters_hold_constant, check_getters, test_getState_mass, and test_getState_mole. The test_isothermal_compressibility could be a good case for a dimensionality check as well.

For PureFluid tests, the test_purefluid.py, in the TestPureFluid class, you can pretty much just copy that class and add units. You don't need to worry about the PureFluidTestCases though, which just test that the numerical results don't change.

Lastly, to actually run the tests (after you do scons build every time you make a change), the command is

python -m unittest -v cantera.test.file_name.class_name.method_name

(making sure to set the PYTHONPATH first) where file_name, class_name, and method_name are all optional. If you omit file_name.class_name.method_name, it runs all the tests. If you omit class_name.method_name, it runs all the tests in the specified file, and so on.

Let me know if you have any questions or run into any problems!

[bryanwweber]

I showed this feature off at a meeting with some other developers yesterday, and they were very impressed! Great work!

One small fix here based on your latest changes 😊

[ischoegl]

To clarify the 'mentioned in #984': there will be a new property YamlWriter.output_units used for serialization. It would make sense to allow for pint units there (eventually).

[bryanwweber]

Hi @hallaali

I squashed the commits, so now there are only 3 on this branch. Remember to do

git fetch origin
git status
git reset --hard origin/units

to incorporate the changes. I made a few small comments for formatting on the examples for you to look at, in addition to the tests. This is very close to being ready for review now!

[bryanwweber]

Some notes after exploring the implementation here a bit:

1. We can use class inheritance for the units.Solution class (inheriting from the upstream Solution), and this will automatically allow us to call properties/methods that aren't defined on our class, without using __setattr__. If we go this route, all the things that are currently self._phase.XXXXX will have to change to super().XXXXX.
2. The disadvantage of the class inheritance approach is that units.Solution cannot be subclassed. This applies more so to the units.PureFluid case. If units.PureFluid inherits from upstream PureFluid, then subclasses of units.PureFluid do not pass their __init__ arguments to the __cinit__ method of the upstream PureFluid class.
3. That point suggests that the current approach of having an instance attribute pointing to the "super" class (that is, upstream Solution or PureFluid) may be more flexible. What we found in using the instance attribute is that we couldn't set attributes on the instance attribute unless we added a __setattr__ to units.Solution. However, creating __setattr__ that sets attributes on the instance attribute led to an infinite recursion, since we also add a __getattr__ method. The solution to the recursion is to directly add the instance attribute to the __dict__ attribute of the class.

I think my personal preference is for option 3, given the constraints of option 2.

[speth]

The disadvantage of the class inheritance approach is that units.Solution cannot be subclassed.

Well, it's not that they can't be subclassed, it's just that the arguments passed to the derived class constructor go straight to __cinit__ without any modification, so the ways in which you can change the constructor arguments are somewhat limited.

I realized that the same problem exists for the existing PureFluid class as well. The way we get around it is that the specific pure fluid instantiators aren't classes, they're just simple factory functions:

def Methane():
    return PureFluid('liquidvapor.yaml', 'methane')

And that solution (sorry) would work here, too.

[bryanwweber]

@speth Ah, wonderful! We'll go with the class inheritance method then.

[bryanwweber]

@speth OK, further tries with class inheritance. I can't figure out how to set properties on the superclass. The naive approach of:

from .. import Solution as _Solution

class Solution(_Solution)
    def __init__(self, infile, phasename=""):
        super().__init__(infile, phasename)

    @property
    def TP(self):
        T, P = super().TP
        return Q_(T, "K"), Q_(P, "Pa")

    @TP.setter
    def TP(self, value):
        T = value[0].to("K") if value[0] is not None else self.T
        P = value[1].to("Pa") if value[1] is not None else self.P
        super().TP = T.magnitude, P.magnitude

works fine to get the property, but super() cannot be used to set instance attributes on the super class, it gives an error:

---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-5-63d859b15053> in <module>
----> 1 gas.TP = 500 * ctu.units.K, None

~/GitHub/cantera/build/python/cantera/units/solution.py in TP(self, value)
    250         T = value[0].to("K") if value[0] is not None else self.T
    251         P = value[1].to("Pa") if value[1] is not None else self.P
--> 252         super().TP = T.magnitude, P.magnitude
    253
    254     @property

AttributeError: 'super' object has no attribute 'TP'

See: https://stackoverflow.com/a/15989250 Other suggested solutions, like directly using the super class name (_Solution.TP) don't raise an exception, but also don't actually change the superclass properties. Plus, doing that same approach with _PureFluid.TP gives an error:

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-4-9817b8aa1512> in <module>
----> 1 w.TQ = None, None

~/GitHub/cantera/build/python/cantera/units/solution.py in TQ(self, value)
   1157         T = value[0].to("K") if value[0] is not None else self.T
   1158         Q = value[1].to("dimensionless") if value[1] is not None else self.Q
-> 1159         _PureFluid.TQ = T.magnitude, Q.magnitude
   1160
   1161     @property

TypeError: can't set attributes of built-in/extension type 'cantera._cantera.PureFluid'

which is presumably not raised for the _Solution case because that class is defined in a .py file rather than a Cython file.

Edit to add: Last, trying to set self.XXXX results in a recursion error, which is expected. I think the reason we can't do this approach with these classes, whereas they work for the base Solution class is because we need to override the getters and setters to add or remove units from the values.

So, as suggested in that Stackoverflow link, I'm inclined to go back to setting an instance attribute on these classes to hold an instance of the "super class", and use __get/setattr__ to retrieve undefined properties.

[bryanwweber]

@hallaali If you think this is good now, I think it's ready for review, so you can hit the button to mark it ready!

[hallaali]

Sounds good, I'll do that now!

…

On Mon, May 3, 2021 at 5:24 PM Bryan W. Weber ***@***.***> wrote: *Message sent from a system outside of UConn.* @hallaali <https://github.com/hallaali> If you think this is good now, I think it's ready for review, so you can hit the button to mark it ready! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#991 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ASSB5U2CUZT5Z3UJWNDUOXDTL4IAPANCNFSM4ZC4Z32A> .

Stale review

[bryanwweber]

There's just a lot of redundant information with no added value.

@ischoegl Can you describe what you'd like to see as far as documentation then? How else should we document this interface?

@speth I think I addressed all the review comments. Please let me know if there's any more I can do here!

[speth]

This all looks good to me. I think there are just two last things:

• @bryanwweber - Can you add Hallaali to the AUTHORS file?
• @ischoegl - Would adding :no-members: to the two new classes to suppress all the duplicated member function / property documentation address your concern about the redundancy of the documentation? This way, the new classes would just have their class docstrings in the Sphinx docs, which do include some content specific to this interface.

[ischoegl]

• @ischoegl - Would adding :no-members: to the two new classes to suppress all the duplicated member function / property documentation address your concern about the redundancy of the documentation? This way, the new classes would just have their class docstrings in the Sphinx docs, which do include some content specific to this interface.

@speth … yes, I was about to suggest just that. That way, Sphinx documents where units are available, without creating unnecessary (and confusing) information.

[bryanwweber]

Awesome, thanks guys! I'll take care of both of those tonight

[bryanwweber]

@speth there's a segfault on the Python 3.8 tests, I'm not sure why. Do you have any immediate insight? I'm not sure why it's just one platform, hope it's not due to any of the changes I made to build-related stuff

[speth]

It's failing somewhere in the ExtensibleReaction tests, which this PR doesn't touch at all. I think there may be an error in my recent PR for those (#1478) that is causing sporadic problems, but nothing to worry about here.

[bryanwweber]

@ischoegl @speth I added a little more text to link to examples and other docs. Let's get this merged, please 😄