-
Notifications
You must be signed in to change notification settings - Fork 15
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
Fix Celcius Behavior #114
base: main
Are you sure you want to change the base?
Fix Celcius Behavior #114
Conversation
Sorry, I checked this against the chemked file I was working with, and didn't have any problems. But, my changes don't pass other tests. I should have run all tests before the pull request. Now I know how to run travis tests without a pull request for next time... Anyway, I'll work on it tomorrow to make this more generally applicable so that it can pass the tests. |
Codecov Report
@@ Coverage Diff @@
## master #114 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 4 4
Lines 966 975 +9
Branches 226 226
=====================================
+ Hits 966 975 +9
Continue to review full report at Codecov.
|
I altered my change so that it only applies when dealing with celsius and fahrenheit units, to avoid issues with "unusual" units, like |
@jsantner Rather than reverting commits, it would be better to rebase and drop them. Then you can |
84407c8
to
e07c89a
Compare
Previously, code would interpret a value in Celsius, such as `50 degC` as `50 * 1 degC`. However, relative quantities like Celcius and Fahrenheit cannot be multiplied. This commit fixes the problem.
e07c89a
to
3681791
Compare
@bryanwweber I had been using GitHub Desktop, which doesn't seem to allow rebasing. I have the command-line interface for git now, and rebased as you asked, with some minor hiccups caused by conflicts between the command line and desktop. |
pyked/chemked.py
Outdated
@@ -718,7 +718,16 @@ def __init__(self, properties): | |||
def process_quantity(self, properties): | |||
"""Process the uncertainty information from a given quantity and return it | |||
""" | |||
quant = Q_(properties[0]) | |||
if type(properties[0]) is str: |
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 the more simpler way of doing this would be to try
to create the Quantity, catch
the error, split and create the Quantity again, and let it raise if that still fails. Something like
try:
quant = Q_(properties[0])
except pint.OffsetUnitCalculusError:
quant = Q_(*properties[0].split())
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 made those changes, with some slight modifications.
Q_(*properties[0].split())
won't work with an offset unit when the quantity is later converted to the base units, so I have to do the less elegant method where the first value is converted to a float. For example:
test = Q_('50', 'degC') # This does not raise any error message
test.to('kelvin') # This raises a TypeError
Q_(50.0, 'degC').to('kelvin') # This doesn't raise any errors
I also added code to handle nan's. Although it isn't really in the scope of this pull request, it deals with the same section of code. I've found some yaml files where a field is given as
ignition-delay:
- nan ms
I've added code to handle this case properly by replacing the nan
string with np.nan
. There may be other errors in yaml files that raise pint.UndefinedUnitError
, so I included an if statement to exclude those.
I didn't know if there was a simpler way to import the two errors from pint, so I had to import all of pint. Is there a cleaner way to import the errors?
Simplify handling of Offset Units. Also, add functionality for 'nan' in yaml files.
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.
Just some clarification on why we need the UndefinedUnitError here
pyked/chemked.py
Outdated
quant = Q_(float(values[0]), ''.join(values[1:])) | ||
except pint.UndefinedUnitError: | ||
values = properties[0].split() | ||
if values[0] == 'nan': |
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.
In what situations do we end up in this branch? Why are we checking for nan specifically? All the values should be filled in from the YAML file, as far as I know. Any branches added here (if/else statements) need tests for both sides of the statement.
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've seen some cases where the yaml file will have a value given as nan
. In that case, pint gives the UndefinedUnitError
and we end up in this branch, which will replace an ignition delay of Q_('nan ms')
with Q_(np.nan, 'ms')
. I added the if/else statements because there are other issues that raise this error. Note that I made a second commit (5c74873) that raises the UndefinedUnitError
if it really is an undefined unit (as in a typo, Q_('50.2 mms')
)
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.
@kyleniemeyer What are your thoughts on nan being specified in the yaml? I think those values should just not be specified
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 do not think we should allow nan in the YAML files—I can't really see a case where that would be necessary. If something wasn't detected (... first-stage ignition, maybe?) then the field should just not be specified.
@jsantner can you give a little more detail where you saw nan in use? Just because someone put it in doesn't mean we should support it... since currently that isn't part of the standard.
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.
In one of the RCM workshop yaml files, ignition delay was given as nan. I'm guessing somebody wrote a script to automatically find ignition delay and create the yaml file, and didn't catch an error in their ignition delay determination. Since you both don't think pyked should allow nan's, then the UndefinedUnitError doesn't need to be excepted here - it will be raised, and that should show a user that their 'nan' is a problem. I just made a commit to remove this block excepting UndefinedUnitError
quant = Q_(properties[0]) | ||
except pint.OffsetUnitCalculusError: | ||
values = properties[0].split() | ||
quant = Q_(float(values[0]), ''.join(values[1:])) |
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.
Good catch, if there are multiple units, this will catch that.
Celcius (and Fahrenheit) temperatures were not read correctly, creating errors. Previously, a temperature such as
50 degC
would be read by multiplying50
anddegC
, giving an error because a relative unit likedegC
cannot be multiplied. This pull request fixes the issue, and adds a test involving Celsius. I created a new .yaml file for this test - should I modify one of the existing files, liketestfile_rcm.yaml
, instead?I apologize for the messy commits and reversions of commits. I was working on a separate issue at the same time, but that issue deserves more work (and its own pull request) so I reverted those commits.
@pr-omethe-us/chemked