Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cantera Units #991

Merged
merged 41 commits into from
Apr 25, 2023
Merged

Cantera Units #991

merged 41 commits into from
Apr 25, 2023

Conversation

hallaali
Copy link
Contributor

@hallaali hallaali commented Mar 12, 2021

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

Copy link
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

interfaces/cython/cantera/units/solution.py.in Outdated Show resolved Hide resolved
interfaces/cython/SConscript Outdated Show resolved Hide resolved
interfaces/cython/SConscript Outdated Show resolved Hide resolved
interfaces/cython/SConscript Outdated Show resolved Hide resolved
interfaces/cython/SConscript Outdated Show resolved Hide resolved
interfaces/cython/SConscript Outdated Show resolved Hide resolved
interfaces/cython/cantera/units/__init__.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/units/solution.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/units/solution.py.in Outdated Show resolved Hide resolved
interfaces/cython/cantera/units/solution.py.in Outdated Show resolved Hide resolved
Copy link
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😊

interfaces/cython/cantera/units/solution.py.in Outdated Show resolved Hide resolved
@bryanwweber

This comment was marked as outdated.

@ischoegl

This comment was marked as outdated.

@bryanwweber

This comment was marked as resolved.

@ischoegl

This comment was marked as resolved.

@bryanwweber

This comment was marked as resolved.

@ischoegl

This comment was marked as resolved.

@bryanwweber

This comment was marked as resolved.

@ischoegl

This comment was marked as resolved.

@ischoegl
Copy link
Member

ischoegl commented Apr 10, 2021

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

Copy link
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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
Copy link
Member

speth commented Apr 29, 2021

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
Copy link
Member

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

@bryanwweber
Copy link
Member

bryanwweber commented Apr 30, 2021

@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
Copy link
Member

@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 hallaali closed this May 4, 2021
@hallaali hallaali reopened this May 4, 2021
@hallaali
Copy link
Contributor Author

hallaali commented May 4, 2021 via email

@hallaali hallaali marked this pull request as ready for review May 4, 2021 01:41
@bryanwweber bryanwweber requested a review from a team May 4, 2021 01:52
@bryanwweber bryanwweber dismissed their stale review May 4, 2021 01:53

Stale review

bryanwweber and others added 11 commits April 22, 2023 14:37
Most of the docs are copied from the upstream object. Some updates are
included for units.
This is required to find the version of Cantera in the dist folder. By
default, pip only finds stable versions, and in this case, was going out
to PyPI to find Cantera rather than use the local artifact. However, we
can't set --no-index because we also need to install other dependencies
from PyPI.
Ensure that we only install the wheel from the archive and don't
accidentally reach out to PyPI.
Co-authored-by: Ray Speth <yarmond@gmail.com>
@bryanwweber
Copy link
Member

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!

Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

  • @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
Copy link
Member

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

@bryanwweber
Copy link
Member

@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
Copy link
Member

speth commented Apr 24, 2023

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
Copy link
Member

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

@speth speth merged commit 32be053 into Cantera:main Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

4 participants