-
-
Notifications
You must be signed in to change notification settings - Fork 361
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
Fix yaml duplicate reaction check #866
Fix yaml duplicate reaction check #866
Conversation
Codecov Report
@@ Coverage Diff @@
## master #866 +/- ##
==========================================
- Coverage 70.49% 70.49% -0.01%
==========================================
Files 375 375
Lines 45649 45671 +22
==========================================
+ Hits 32182 32197 +15
- Misses 13467 13474 +7
Continue to review full report at Codecov.
|
22f7f69
to
83f2c45
Compare
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.
Thanks for fixing this @speth! Just a few small things I noticed.
This form is useful for errors that involve two locations within the input file, such as duplicate reactions.
83f2c45
to
93a6e84
Compare
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.
Thanks, looks good now! I have a question below which is partly for my understanding, but may be worth adding a comment in the code for future us as well, depending on the answer.
if match: | ||
reactions.append(int(match.group(1))-1) | ||
|
||
if len(reactions) != 2: |
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.
Why is this check necessary? From my quick test, adding a third duplicate reaction still only shows two of them. Are there other error conditions that can reach this function?
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.
Right, the error from Kinetics::checkDuplicates
should only ever indicate two reactions. What I'm trying to hedge against is (a) the possibility there's some case where that error message doesn't have the expected format, or (b) some future change breaks this attempt at parsing the error message. If either of those happens, I'd rather output the original message from Kinetics::checkDuplicates
than some error message generated caused by reactions
not having the the expected number of entries.
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.
I added some comments to show_duplicate_reactions
which will hopefully make some of this clear to our future selves. 😂
Use the more comprehensive duplicate reaction check in Kinetics::checkDuplicates when validating the mechanism. Parse the error message generated by Kinetics::checkDuplicates to determine the line number in the original input file, which is more useful to users than the line number in the newly-generated YAML file.
93a6e84
to
1e1340b
Compare
Changes proposed in this pull request
ck2yaml.py
with the full one done in the C++Kinetics
class, and parse the output of that error to provide line numbers of the duplicate reactions in the original input file, e.g.:AnyValue
andAnyMap
to make this a bit easier.Checklist
scons build
&scons test
) and unit tests address code coverage