-
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
[WIP] Add time histories #104
[WIP] Add time histories #104
Conversation
No validation yet
With existing RCM files
Remove it after we release PyKED 0.4
Includes stroke, clearance, and compression ratio. Move compressed_pressure, compressed_temperature, and compression_time to rcm-data field. Work on #91
Was this expected to fail (if still in progress), or no? |
It's a flake8 failure, so I'd say, yes because WIP, but not expected 😄 |
pyked/tests/testfile_rcm2.yaml
Outdated
clearance: | ||
- 2.5 cm | ||
compression-ratio: | ||
- '12.0' |
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.
are the quote marks needed here?
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.
Ah, no, shouldn't be, I was testing a weird failure mode. Thanks for the catch
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.
Looks good, other than my few comments
pyked/validation.py
Outdated
def _validate_isvalid_quantity(self, isvalid_quantity, field, value): | ||
"""Checks for valid given value and appropriate units. | ||
|
||
Args: | ||
isvalid_quantity (`bool`): flag from schema indicating quantity to be checked. | ||
field (`str`): property associated with quantity in question. | ||
value (`str`): string of the value of the quantity | ||
value (`list`): list with the first element a string of value with units |
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.
This doesn't read correctly to me: list with the first element a string of value with units
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.
Oh yeah, that's really bad grammar hah
These properties are necessary to reconstruct a volume trace from a pressure trace in an RCM experiment.
Codecov Report
@@ Coverage Diff @@
## master #104 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 3 3
Lines 878 951 +73
Branches 212 228 +16
=====================================
+ Hits 878 951 +73
Continue to review full report at Codecov.
|
Once the tests pass, this should be good to merge 👍 |
CHANGELOG.md
Changes proposed in this pull request:
time-histories
field to encapsulate all of the time-varying properties of an experiment, including pressure, temperature, volume, absorption, emission, and piston position@pr-omethe-us/chemked