Fix Celcius Behavior

[jsantner]

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]

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]

Codecov Report

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

@@          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]

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]

@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]

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

[bryanwweber]

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

[jsantner]

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?

[bryanwweber]

Just some clarification on why we need the UndefinedUnitError here

[bryanwweber]

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.

[jsantner]

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

[bryanwweber]

@kyleniemeyer What are your thoughts on nan being specified in the yaml? I think those values should just not be specified

[kyleniemeyer]

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.

[jsantner]

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

[bryanwweber]

Good catch, if there are multiple units, this will catch that.