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

Chemkin files with weird names break PDep reactions. #460

Merged
merged 3 commits into from
Jul 12, 2017

Conversation

rwest
Copy link
Member

@rwest rwest commented Jun 29, 2017

The first commit in this PR just contains a failing unit test case.
I have not figured out the solution, but thought this would perhaps be more helpful than just opening an issue. Please feel free to add a fix rather than waiting for me to finish the PR :-)

Reactions of the type
A (+B) <=> C (+B)
ought to work, as long as they are provided a pressure-dependent rate
expression. This commit adds three examples to the test file for
pathological species names. The first works OK (third body is called "plus"), the second two cause problems (third body is "(Parens)" or "plus+")

(For what it's worth, this currently crashes the official chemkin.
Or at least the parentheses do; I've not tested the plus.
Ansys have created a defect record and say they will fix the issue.)

Reactions of the type
 A (+B) <=> C (+B)
ought to work, as long as they are provided a pressure-dependent rate
expression. This commit adds three examples to the test file. The first
works OK, the second two cause problems.

(For what it's worth, this currently crashes the official chemkin.
 Or at least the parentheses do; I've not tested the plus.
 Ansys have created a defect record and say they will fix the issue.)
@rwest
Copy link
Member Author

rwest commented Jul 7, 2017

@alongd reports that Ansys / Reaction Design claim to have fixed the parenthesis issue in Chemkin and the fix will be included in the upcoming Chemkin release. (Case Number: 23852, fix: DE153361)

This changes the order in which tokens are identified to be strictly
descending in length, so that third bodies are identified correctly
even when the third body expression could potentially be interpreted
as containing a standalone species name.
Replace the heuristic used to remove the third body terms from the
reactant and product lists to handle species names that include
parentheses.
@speth
Copy link
Member

speth commented Jul 12, 2017

When I first saw the title of this, I thought there was a processing problem based on the filename, and was really confused. But this looks like just another issue caused by the fact that the Chemkin format doesn't have (and may not be able to have) a clearly defined grammar. In any case, I think this issue should be fixed now.

@rwest
Copy link
Member Author

rwest commented Jul 12, 2017

Awesome. Thanks Ray. Yes, I can see the confusion with my issue title.

@codecov
Copy link

codecov bot commented Jul 12, 2017

Codecov Report

Merging #460 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #460      +/-   ##
==========================================
+ Coverage   58.23%   58.25%   +0.01%     
==========================================
  Files         386      386              
  Lines       42310    42355      +45     
  Branches     7252     7267      +15     
==========================================
+ Hits        24640    24672      +32     
  Misses      15488    15488              
- Partials     2182     2195      +13
Impacted Files Coverage Δ
src/kinetics/importKinetics.cpp 76.61% <0%> (-4.16%) ⬇️
src/kinetics/ReactionPath.cpp 71.88% <0%> (-0.06%) ⬇️
src/kinetics/Reaction.cpp 66.48% <0%> (ø) ⬆️
src/kinetics/InterfaceKinetics.cpp 67.13% <0%> (+0.23%) ⬆️

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 6bf74d1...20e67da. Read the comment docs.

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.

2 participants