Skip to content

Fix Celcius Behavior #114

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.

Sign up for GitHub

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

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

Fix Celcius Behavior #114

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1cee8b9
Correctly read Celsius temperatures
jsantner Jun 14, 2018
cfdbae2
Add test with temperature in Celsius
jsantner Jun 14, 2018
2c0b278
Minor fix in arguments
jsantner Jun 14, 2018
73551ab
Only apply to celsius and fahrenheit
jsantner Jun 14, 2018
3681791
Syntax
jsantner Jun 14, 2018
f759ab1
Fix behavior in chemked.process_quantity
jsantner Jun 14, 2018
2147780
Fixing changes that were requested.
jsantner Jun 25, 2018
5c74873
raise pint.UndefinedUnitError if it can't be fixed by np.nan
jsantner Jun 25, 2018
ae14c43
Remove 'nan' handling
jsantner Jun 26, 2018
befbe89
Add test for changes in chemked.py
jsantner Jul 10, 2018
6eee6df
pint quantities must be in same unit to be compared with np.isclose
jsantner Jul 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion pyked/chemked.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import xml.etree.ElementTree as etree
import xml.dom.minidom as minidom
from itertools import chain
import pint

import numpy as np

Expand Down Expand Up @@ -718,7 +719,17 @@ def __init__(self, properties):
def process_quantity(self, properties):
"""Process the uncertainty information from a given quantity and return it
"""
quant = Q_(properties[0])
try:
quant = Q_(properties[0])
except pint.OffsetUnitCalculusError:
values = properties[0].split()
quant = Q_(float(values[0]), ''.join(values[1:]))

@bryanwweber bryanwweber Jun 25, 2018

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

except pint.UndefinedUnitError:
values = properties[0].split()
if values[0] == 'nan':

@bryanwweber bryanwweber Jun 25, 2018

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

@jsantner jsantner Jun 25, 2018

Copy link
Copy Markdown
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'))

@bryanwweber bryanwweber Jun 26, 2018

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

@kyleniemeyer kyleniemeyer Jun 26, 2018

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

@jsantner jsantner Jun 26, 2018 •
edited
Loading

Copy link
Copy Markdown
Author

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_(np.nan, ''.join(values[1:]))
else:
raise
if len(properties) > 1:
unc = properties[1]
uncertainty = unc.get('uncertainty', False)
Expand Down
2 changes: 1 addition & 1 deletion pyked/tests/test_validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ def properties(self, request):

@pytest.mark.parametrize("properties", [
'testfile_st.yaml', 'testfile_st2.yaml', 'testfile_rcm.yaml', 'testfile_required.yaml',
'testfile_uncertainty.yaml', 'testfile_rcm2.yaml',
'testfile_uncertainty.yaml', 'testfile_rcm2.yaml', 'testfile_rcm3.yaml'
], indirect=['properties'])
def test_valid_yaml(self, properties):
"""Ensure ChemKED YAML is validated
Expand Down
178 changes: 178 additions & 0 deletions pyked/tests/testfile_rcm3.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,178 @@
---
file-authors:
- name: Kyle E Niemeyer
ORCID: 0000-0003-4425-7097
file-version: 0
chemked-version: 0.0.1
reference:
doi: 10.1002/kin.20180
authors:
- name: Gaurav Mittal
- name: Chih-Jen Sung
ORCID: 0000-0003-2046-8076
- name: Richard A Yetter
journal: International Journal of Chemical Kinetics
year: 2006
volume: 38
pages: 516-529
detail: Fig. 6, open circle
experiment-type: ignition delay
apparatus:
kind: rapid compression machine
institution: Case Western Reserve University
facility: CWRU RCM
datapoints:
- temperature:
- 24.25 degC
ignition-delay:
- 1.0 ms
first-stage-ignition-delay:
- 0.5 ms
- uncertainty-type: relative
uncertainty: 0.01
pressure:
- 958.0 torr
rcm-data:
compressed-pressure:
- 7.1 bar
compressed-temperature:
- 765 K
- uncertainty: 0.01
uncertainty-type: relative
compression-time:
- 38.0 ms
stroke:
- 10.0 inch
clearance:
- 2.5 cm
compression-ratio:
- 12.0
composition:
kind: mole fraction
species:
- species-name: H2
InChI: 1S/H2/h1H
amount:
- 0.12500
- species-name: O2
InChI: 1S/O2/c1-2
amount:
- 0.06250
- species-name: N2
InChI: 1S/N2/c1-2
amount:
- 0.18125
- species-name: Ar
InChI: 1S/Ar
amount:
- 0.63125
ignition-type:
target: pressure
type: d/dt max
time-histories:
- type: volume
time:
units: s
column: 0
quantity:
units: cm3
column: 1
values:
- [0.00E+000, 5.47669375000E+002]
- [1.00E-003, 5.46608789894E+002]
- [2.00E-003, 5.43427034574E+002]
- [3.00E-003, 5.38124109043E+002]
- [4.00E-003, 5.30700013298E+002]
- [5.00E-003, 5.21154747340E+002]
- [6.00E-003, 5.09488311170E+002]
- [7.00E-003, 4.95700704787E+002]
- [8.00E-003, 4.79791928191E+002]
- [9.00E-003, 4.61761981383E+002]
- [1.00E-002, 4.41610864362E+002]
- [1.10E-002, 4.20399162234E+002]
- [1.20E-002, 3.99187460106E+002]
- [1.30E-002, 3.77975757979E+002]
- [1.40E-002, 3.56764055851E+002]
- [1.50E-002, 3.35552353723E+002]
- [1.60E-002, 3.14340651596E+002]
- [1.70E-002, 2.93128949468E+002]
- [1.80E-002, 2.71917247340E+002]
- [1.90E-002, 2.50705545213E+002]
- [2.00E-002, 2.29493843085E+002]
- [2.10E-002, 2.08282140957E+002]
- [2.20E-002, 1.87070438830E+002]
- [2.30E-002, 1.65858736702E+002]
- [2.40E-002, 1.44647034574E+002]
- [2.50E-002, 1.23435332447E+002]
- [2.60E-002, 1.02223630319E+002]
- [2.70E-002, 8.10119281915E+001]
- [2.80E-002, 6.33355097518E+001]
- [2.90E-002, 5.27296586879E+001]
- [3.00E-002, 4.91943750000E+001]
- [3.10E-002, 4.97137623933E+001]
- [3.20E-002, 5.02063762048E+001]
- [3.30E-002, 5.06454851923E+001]
- [3.40E-002, 5.10218564529E+001]
- [3.50E-002, 5.13374097598E+001]
- [3.60E-002, 5.16004693977E+001]
- [3.70E-002, 5.18223244382E+001]
- [3.80E-002, 5.20148449242E+001]
- [3.90E-002, 5.21889350372E+001]
- [4.00E-002, 5.23536351113E+001]
- [4.10E-002, 5.25157124459E+001]
- [4.20E-002, 5.26796063730E+001]
- [4.30E-002, 5.28476160610E+001]
- [4.40E-002, 5.30202402028E+001]
- [4.50E-002, 5.31965961563E+001]
- [4.60E-002, 5.33748623839E+001]
- [4.70E-002, 5.35527022996E+001]
- [4.80E-002, 5.37276399831E+001]
- [4.90E-002, 5.38973687732E+001]
- [5.00E-002, 5.40599826225E+001]
- [5.10E-002, 5.42141273988E+001]
- [5.20E-002, 5.43590751578E+001]
- [5.30E-002, 5.44947289126E+001]
- [5.40E-002, 5.46215686913E+001]
- [5.50E-002, 5.47405518236E+001]
- [5.60E-002, 5.48529815402E+001]
- [5.70E-002, 5.49603582190E+001]
- [5.80E-002, 5.50642270863E+001]
- [5.90E-002, 5.51660349836E+001]
- [6.00E-002, 5.52670070646E+001]
- [6.10E-002, 5.53680520985E+001]
- [6.20E-002, 5.54697025392E+001]
- [6.30E-002, 5.55720927915E+001]
- [6.40E-002, 5.56749762728E+001]
- [6.50E-002, 5.57777790517E+001]
- [6.60E-002, 5.58796851466E+001]
- [6.70E-002, 5.59797461155E+001]
- [6.80E-002, 5.60770054561E+001]
- [6.90E-002, 5.61706266985E+001]
- [7.00E-002, 5.62600130036E+001]
- [7.10E-002, 5.63449057053E+001]
- [7.20E-002, 5.64254496625E+001]
- [7.30E-002, 5.65022146282E+001]
- [7.40E-002, 5.65761642150E+001]
- [7.50E-002, 5.66485675508E+001]
- [7.60E-002, 5.67208534842E+001]
- [7.70E-002, 5.67944133373E+001]
- [7.80E-002, 5.68703658198E+001]
- [7.90E-002, 5.69493069272E+001]
- [8.00E-002, 5.70310785669E+001]
- [8.10E-002, 5.71146023893E+001]
- [8.20E-002, 5.71978399741E+001]
- [8.30E-002, 5.72779572372E+001]
- [8.40E-002, 5.73517897984E+001]
- [8.50E-002, 5.74167271960E+001]
- [8.60E-002, 5.74721573687E+001]
- [8.70E-002, 5.75216388520E+001]
- [8.80E-002, 5.75759967785E+001]
- [8.90E-002, 5.76575701358E+001]
- [9.00E-002, 5.78058719368E+001]
- [9.10E-002, 5.80849611077E+001]
- [9.20E-002, 5.85928651155E+001]
- [9.30E-002, 5.94734357453E+001]
- [9.40E-002, 6.09310671165E+001]
- [9.50E-002, 6.32487551103E+001]
- [9.60E-002, 6.68100309742E+001]
...
12 changes: 11 additions & 1 deletion pyked/validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,17 @@ def _validate_isvalid_quantity(self, isvalid_quantity, field, value):
{'isvalid_quantity': {'type': 'bool'}, 'field': {'type': 'str'},
'value': {'type': 'list'}}
"""
quantity = Q_(value[0])
try:
quantity = Q_(value[0])
except pint.OffsetUnitCalculusError:
values = value[0].split()
quantity = Q_(float(values[0]), ''.join(values[1:]))
except pint.UndefinedUnitError:
values = value[0].split()
if values[0] == 'nan':
quantity = Q_(np.nan, ''.join(values[1:]))
else:
raise
low_lim = 0.0 * units(property_units[field])

try:
Expand Down