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

Methyl heptane #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Methyl heptane #19

wants to merge 1 commit into from

Conversation

ajuluc
Copy link

@ajuluc ajuluc commented Oct 16, 2018

No description provided.

@rwest
Copy link
Member

rwest commented Oct 17, 2018

This includes changes to methyl decanoate, and a lot of fix up commits. Please could you interactive rebase to tidy it up? Thanks Zil

@ajuluc
Copy link
Author

ajuluc commented Oct 17, 2018

Yeah sure I'm working on it at the moment

@bryanwweber
Copy link
Member

Is this PR ready for review? @nateharms @ajuluc

@nateharms
Copy link

Yep

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.

Thank you for submitting this data! There seem to be a few problems with it, can you address the comments below? Thank you!

@@ -0,0 +1,148 @@
---
file-authors:
- name: Nathan Harms
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indentation should be 2 spaces for YAML files

- name: Nathan Harms
ORCID: 0000-0003-2680-371X
file-version: 0
chemked-version: 0.0.1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chemked-version should be 0.4.1 if these files are compatible with that version (if they're not, they should be made to be).

- temperature:
- 703.0 kelvin
ignition-delay:
- 47.0 us
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the ignition delay is the same as the first data point (its also the same for all the points in this file and a few others). Can you please check this?

composition: *comp
ignition-type: *ign
equivalence-ratio: 0.5

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure to end the file with a newline. Editors such as Atom and VSCode can be configured to add this automatically 😄

ignition-delay:
- 47.0 us
- uncertainty-type: relative
uncertainty: 0.2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the uncertainty really 20%? Seems high, but if its from the paper, that's fine.

datapoints:
- temperature:
- 373.0 kelvin
compressed_temperature:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we have a new(er) rcm-data field, see https://pr-omethe-us.github.io/PyKED/schema-docs.html#ignition-rcm-data All the RCM-specific data should go in that field

composition: *comp
ignition-type: *ign
equivalence-ratio: 0.5

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Newline

composition: *comp
ignition-type: *ign
equivalence-ratio: 1.0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Newline

- temperature:
- 678.0 kelvin
ignition-delay:
- 47.0 us
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file also has all 47.0 us as the ignition delay

- temperature:
- 692.0 kelvin
ignition-delay:
- 47.0 us
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is also all 47.0 us as the ignition delay

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.

4 participants