Codecov Report

Merging #1076 (6f8aed6) into main (e77148b) will increase coverage by 0.03%.
The diff coverage is 88.65%.

❗ Current head 6f8aed6 differs from pull request most recent head 1844332. Consider uploading reports for the commit 1844332 to get more accurate results

@@            Coverage Diff             @@
##             main    #1076      +/-   ##
==========================================
+ Coverage   73.45%   73.49%   +0.03%     
==========================================
  Files         364      364              
  Lines       47611    47735     +124     
==========================================
+ Hits        34972    35081     +109     
- Misses      12639    12654      +15     

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 e77148b...1844332. Read the comment docs.

@speth ... thanks for your comments!

I think making information about the units of reaction rate coefficients and phase standard concentrations available is a valuable addition to the Python interface, and a light interface to the Units class seems to serve this purpose, though I think there are currently a few quirks. [...]

This distinction gets even muddier with newly-allowed expressions like Units("4 atm").

Interesting observation. I definitely didn't have that in mind when I created the constructor! While I did code this up to finally wrap my head around the internal unit conversions (it did help), my main concern was to display what's done internally.

I'm not clear on what the benefit of exposing the UnitSystem class here is. Is the purpose just to make information about the (completely fixed) default unit system available?

Displaying the unit system was certainly one of my main concerns here (it would clear up things once and for all for any user). But at the same time, it would serve a purpose to have it as a stand-alone object that is passed to YamlWriter (see 5751633), where Unitswith conversion factors do make some sense.

I agree that making the Python Units constructor less 'useful' makes sense. This should be a relatively easy tweak to this PR.

@speth ... I reworked some of the interfaces to make things more intuitive. For unit output, the factor is suppressed:

In [1]: import cantera as ct
   ...:    ...: gas = ct.Solution("gri30.yaml")
   ...:    ...: gas.reaction(0)
   ...: 
Out[1]: <ThreeBodyReaction: 2 O + M <=> O2 + M>

In [2]: gas.reaction(0).rate_coeff_units
Out[2]: <Units(m^6 / kmol^2 / s) at 7fd73eb86f30>

In [3]: gas.standard_concentration_units
Out[3]: <Units(kmol / m^3) at 7fd73eb86c90>

The UnitSystem now displays units automatically, and has a more intuitive units property (instead of the internally used C++ defaults, which originates in the pre-existing Units::setDefaults):

In [4]: ct.UnitSystem().units # default unit system
Out[4]: 
{'activation-energy': 'J / kmol',
 'current': 'A',
 'energy': 'J',
 'length': 'm',
 'mass': 'kg',
 'pressure': 'Pa',
 'quantity': 'kmol',
 'temperature': 'K',
 'time': 's'}

In [5]: units = ct.UnitSystem({"quantity": "mol", "length": "cm"}) 

In [6]: units.units # alternate unit system (e.g. input to YamlWriter)
Out[6]:
{'activation-energy': 'J / mol',
 'current': 'A',
 'energy': 'J',
 'length': 'cm',
 'mass': 'kg',
 'pressure': 'Pa',
 'quantity': 'mol',
 'temperature': 'K',
 'time': 's'}

Finally, unit conversions are no longer enabled for unit strings with non-unity conversion factors:

In [6]: ct.Units('Pa')
Out[6]: <Units(kg / m / s^2) at 7f0275f42c90>

In [7]: ct.Units('3 Pa')
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-3-f54d3ab3f8e5> in <module>
----> 1 ct.Units('3 Pa')

/src/interfaces/cython/cantera/units.pyx in cantera._cantera.Units.__cinit__()
     19             self.units = CxxUnits(stringify(name))
     20             if abs(self.units.factor() - 1.) > np.nextafter(0, 1):
---> 21                 raise ValueError(
     22                     "The creation of 'Units' objects that require a conversion "
     23                     "factor\nwith respect to Cantera's default unit system is not "

ValueError: The creation of 'Units' objects that require a conversion factor
with respect to Cantera's default unit system is not supported:
input '3 Pa' is equivalent to '3.0 kg / m / s^2' where the conversion factor is '3.0'

PS: I added a direct UnitSystem based setter YamlWriter::setUnitSystem to clarify my intent.

In [8]: writer = ct.YamlWriter()
   ...: writer.add_solution(ct.Solution("h2o2.yaml"))
   ...: writer.output_units = ct.UnitSystem({"quantity": "mol", "length": "cm"})

(this was possible even before the latest commits, but now there's a direct path for this)

@speth ... thank you for the additional comments, which should be taken care of. Per suggestion, all the formatting flags are now implemented in C++ rather than just for Python. The unit tests were easily taken care of and the documentation is updated. Regarding Unittest::default the direction I am taking requires the steps, as I am starting from the factors and work my way from there.

@speth ... let me know if there's anything else. From my side, I believe this is ready to be merged.

@speth ... thanks for clarifying! I simply cherry-picked your suggestion as it's indeed simpler (I was apparently too focused on the numeric factors ...).