-
Notifications
You must be signed in to change notification settings - Fork 18
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
Cyclopentane #17
base: master
Are you sure you want to change the base?
Cyclopentane #17
Conversation
Hi! 👋 Welcome to ChemKED! Glad to see more data being added. One comment about this PR before I look into the actual data files - it's really hard to tell what's changed here, because there are so many commits with irrelevant messages/data. Can you squash/rebase the commits so that only the cyclopentane data is added in ~1 commit? |
Is this PR read for review? @nateharms @anthonymstohr |
I believe so. @anthonymstohr @benhoare98 Are there any issues with this PR? |
No issues that I’m aware of. All files validated
On Mon, Dec 3, 2018 at 11:40 AM Nate Harms ***@***.***> wrote:
I believe so. @anthonymstohr <https://github.com/anthonymstohr>
@benhoare98 <https://github.com/benhoare98> Are there any issues with
this PR?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#17 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ApN8EsuJbxLoLw77L_049DPsqItH06Olks5u1VQLgaJpZM4Xc-s7>
.
--
Benjamin R. Hoare
Northeastern University '22
B.S. Candidate, Chemical Engineering
[email protected]
781-254-6496
|
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.
The data here looks good, just one formatting comment. Can you just check whether or not there are any uncertainties listed for quantities such as the pressure, temperature, and composition?
cyclopentane/Rashidi 2017/cyclopentane_rashidi_HPST_KAUST_phi_0.5.yaml
Outdated
Show resolved
Hide resolved
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.
@anthonymstohr @benhoare98 Thanks for the update! Still a few comments to fix below.
- 0.784436 | ||
datapoints: | ||
- temperature: | ||
- 1070 kelvin |
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.
These data need uncertainty as well.
reference: | ||
doi: 10.1016/j.combustflame.2017.05.018 | ||
authors: | ||
- name: Mariam J. Al Rashidi |
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.
The authors for this paper are indented too far. This happened for all of the data files from this author.
I obtained experimental data for cyclopentane from this article:
https://onlinelibrary.wiley.com/doi/full/10.1002/kin.20353
The only problem with the data was that the mole fractions didn't add up to exactly one, so I used proportions to slightly edit the given mole fractions to add up to one so the file could be successfully validated. Let me know if that was the right call and if anything else needs to be fixed.
Thanks