-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
cti2yaml: fix issues with composition strings and kinetics model of "None" #1127
Conversation
This change allows for composition specifications that include a whitespace after a separating colon (example: "N2: 0.9, O2: 0.1" rather than "N2:0.9, O2:0.1")
@speth / @bryanwweber ... I dropped the 'fix' for #1132 from here (per @speth's comment this will resolve itself). |
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, @ischoegl. The first two changes look fine to me.
I'm not sure about the change to "allow for specification of exact reaction id's", though. If I'm reading this correctly, it only supports the case where a single reaction id is given (which I don't think is frequently used), correct? As currently implemented, this has the side-effect of obscuring the warning that was in place to let users know that cti2yaml
does not support the range notation that CTI used (which I think was also rarely used).
" files not supported ({}: {})".format(datasrc, rnum)) | ||
else: | ||
_printerr("WARNING: Reaction specification" | ||
" '{}' not supported".format(rnum)) |
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.
Changing this set of allowed reaction specifications to be permissive means that certain cases that we know are not handled, like n005 to n008
are passed through without raising a warning, and the resulting error when importing the mechanism is not easy to diagnose as it no longer mentions the unsupported user input.
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.
If I'm reading this correctly, it only supports the case where a single reaction id is given
this is correct. I didn’t see a way around replacing the warnings as I had assumed that correct cti conversion was most important.
Could you be more specific about n005 to n008
?
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.
Actually, there's always the option to still produce a warning while leaving the same in place. I added this as a separate commit, but can squash if necessary. As is, the conversion works, but produces a warning, i.e.
$ cti2yaml Mayur-ISSP.cti
WARNING: Conversion of reaction specification 'rxn-SEI-comp' may not be supported
Edit: I ended up changing things, so this is obsolete
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.
Take a look at some of the examples given here: https://cantera.org/tutorials/cti/phases.html#declaring-the-reactions
Admittedly, I've never seen this construct used in the wild, which is why I didn't worry about it for cti2yaml
in the first place. But I've also the reaction selection used to pick just a single reaction, either.
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.
@speth ... thanks! I think it's possible to add the conversion of n005 to n008
if it ever comes up. As it stands, the single reaction already came up, so I'd suggest to update the warning message and leave the import in place.
Edit: I added a separate error for anything containing to
, and tweaked the warning message.
PS: found a better way by catching exceptions at a later point
Codecov Report
@@ Coverage Diff @@
## main #1127 +/- ##
==========================================
+ Coverage 73.45% 73.49% +0.04%
==========================================
Files 365 365
Lines 47912 48187 +275
==========================================
+ Hits 35194 35417 +223
- Misses 12718 12770 +52
Continue to review full report at Codecov.
|
@speth ... thank you for the comment. I believe I found a good solution by raising exceptions when things get parsed at a later point. As an example, the unimplemented
The specification of a single reaction works, and will not raise a confusing warning message (the |
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, @ischoegl. This looks good to me.
Changes proposed in this pull request
transport
/kinetics
models specified asNone
consistentlyN2: 0.9
rather thanN2:0.9
)If applicable, fill in the issue number this pull request is fixing
Closes #1009
If applicable, provide an example illustrating new features this pull request is introducing
see conversion of
Mayur-ISSP.cti
atttached to #1009. The result after applying this fix is Mayur-ISSP.yaml.txtChecklist
scons build
&scons test
) and unit tests address code coverage