-
-
Notifications
You must be signed in to change notification settings - Fork 349
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
Expose C++ Units to Python #1076
Conversation
Codecov Report
@@ 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.
|
08b8e16
to
735990b
Compare
735990b
to
65f1e58
Compare
f25bca8
to
5abd858
Compare
f034688
to
01c2ba1
Compare
01c2ba1
to
6345889
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
While I understand that the string representation of Units
objects draws from the existing implementation (with some cleanup), I think that this output is kind of confusing when the units aren't in Cantera's base system, e.g. in the case of
>>> ct.Units("mol/cm^3")
<Units(999.9999999999999 kmol / m^3) at 7f6ee85c93f0>
it's not really clear anymore that what this is intended to mean is that 1000 is the conversion factor between mol/cm^3 and kmol/m^3, rather than representing a particular quantity of 1000 kmol/m^3. This distinction gets even muddier with newly-allowed expressions like Units("4 atm")
.
Assuming the goal here is just to provide unit information for these special cases where the dimensions vary in different instances, rather than presenting a full unit conversion capability (which I don't think we really want or need) I'd suggest that maybe Python Units
objects should only be constructible in the default unit system, and that they could be represented without the leading 1.0 factor, e.g.
>>> ct.Units("J/kmol")
<Units(kg * m^2 / kmol / s^2) at 7f6ef085c030>
and noting that practically, there's really no purpose for a user-constructed Units
object.
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?
@speth ... thanks for your comments!
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.
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 I agree that making the Python |
Also: remove redundant tests for Units objects.
6345889
to
65dfd08
Compare
@speth ... I reworked some of the interfaces to make things more intuitive. For unit output, the factor is suppressed:
The
Finally, unit conversions are no longer enabled for unit strings with non-unity conversion factors:
PS: I added a direct
(this was possible even before the latest commits, but now there's a direct path for this) |
65dfd08
to
9950057
Compare
589fc5d
to
7a703e7
Compare
7a703e7
to
4a448e8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates, @ischoegl, I think this is fairly close.
23c7b25
to
fd56254
Compare
@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 |
fd56254
to
d45482b
Compare
@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 ...). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @ischoegl, I think that wraps this all up.
Changes proposed in this pull request
Units::str()
outputCxxUnits
to Cython interfaceUnits
Reaction.rate_coeff_units
andThermoPhase.standard_concentration_units
UnitSystem
to PythonIf applicable, provide an example illustrating new features this pull request is introducing
The new feature can be used as follows:
Checklist
scons build
&scons test
) and unit tests address code coverageOther thoughts
This PR seeks to expose units used in the C++ layer, rather than facilitating conversion of alternative units as proposed in #991.