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

Fix Celcius Behavior #114

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

jsantner
Copy link

Celcius (and Fahrenheit) temperatures were not read correctly, creating errors. Previously, a temperature such as 50 degC would be read by multiplying 50 and degC, giving an error because a relative unit like degC 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, like testfile_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

@jsantner
Copy link
Author

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

codecov bot commented Jun 14, 2018

Codecov Report

Merging #114 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #114   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         966    975    +9     
  Branches      226    226           
=====================================
+ Hits          966    975    +9
Impacted Files Coverage Δ
pyked/chemked.py 100% <100%> (ø) ⬆️
pyked/validation.py 100% <100%> (ø) ⬆️

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 57c936c...6eee6df. Read the comment docs.

@jsantner
Copy link
Author

I altered my change so that it only applies when dealing with celsius and fahrenheit units, to avoid issues with "unusual" units, like 999 / seconds. What do you think?

@bryanwweber
Copy link
Member

@jsantner Rather than reverting commits, it would be better to rebase and drop them. Then you can git push -f to this branch to update the PR.

@jsantner jsantner force-pushed the fix-celcius-behavior branch from 84407c8 to e07c89a Compare June 14, 2018 19:27
jsantner added 5 commits June 14, 2018 12:37
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.
@jsantner jsantner force-pushed the fix-celcius-behavior branch from e07c89a to 3681791 Compare June 14, 2018 19:38
@jsantner
Copy link
Author

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

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

Copy link
Author

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?

jsantner added 2 commits June 25, 2018 09:29
Simplify handling of Offset Units. Also, add functionality for 'nan' in yaml files.
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.

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

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.

Copy link
Author

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

Copy link
Member

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

Copy link
Member

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.

Copy link
Author

@jsantner jsantner Jun 26, 2018

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:]))
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants