pr-omethe-us / PyKED

Python interface to the ChemKED database format
https://pr-omethe-us.github.io/PyKED/
BSD 3-Clause "New" or "Revised" License
14 stars 15 forks source link

Fix Celcius Behavior #114

Open jsantner opened 6 years ago

jsantner commented 6 years ago

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 commented 6 years ago

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[bot] commented 6 years ago

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%> (ø) :arrow_up:
pyked/validation.py 100% <100%> (ø) :arrow_up:

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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