-
-
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
YAML: consistently clarify reaction type #747
Comments
For an interface reaction, you do not need to parse the reaction equation to know that it's of that type. It's required by the fact that you're adding it to an For electrochemical reactions, the problem is the case where the reaction involves charge transfer but the reaction isn't explicitly identified as such (see #452). If the user has to specify I think there are still options for extensibility (including user-derived types) even if we use the reaction equation or other information besides the I might even consider it an improvement to go further in this direction and not require the |
@speth ... yes, it is true that things can be deduced, but is this an efficient implementation? If we have a reaction mechanism with 10k+ reactions, it is quicker to simply access a Imho, there are several arguments against trying to eliminate the
PS: I wasn't aware of #452, but would argue that this is a shortcoming in the XML input file format, which imho has better solutions for YAMl. I.e. this leaves |
I guess that in many cases (including three-body), autodetection may not introduce much overhead (I even may take a stab). Still, for |
Agreed, it does simplify the factory class implementation.
Two things: First, I did some benchmarking while developing the YAML parsing code, and I've been pretty happy with the performance so far. It's significantly faster than using the XML format, let alone converting CTI to XML. To provide a bit of data, I just ran a comparison for a fairly large mechanism (LLNL butanol mechanism with 431 species and 2551 reactions) and importing in Python with
The takeaway is that creating a Second, we have to parse the reaction equation at some point. And if the check that we would
I think there are different ways to think about what's more useful to the user. One is that having the
While I agree that we should try to detect any inconsistencies, requiring redundant information to be encoded in the reaction entry just compounds the potential for errors. And I think the setup step for YAML currently does fail for many (I don't want underestimate the creativity of users and say all) of the possible inconsistencies in reaction information.
This is perhaps a bit subtle, but it's impossible to determine the units of the pre-exponential factor, which is needed to do unit conversions, without having the |
@speth ... thanks for the detailed response! Responding where appropriate ...
those number are impressive!
Agreed, but it should be done exactly once. In the current implementation, every surface reaction gets parsed twice due to the omission of an appropriate cantera/src/kinetics/Reaction.cpp Lines 1047 to 1049 in 719cb3d
Imho, not setting an electrochemical type in the YAML specification is an omission that complicates the code. Adopting a type will require a transition with some warnings, but long-term there won't be an issue with historical work-arounds.
In the current implementation,
I am pretty unapologetic about this, as it's better to have repeated failures due to erroneous user input than have Cantera incorrectly guessing what a user had in mind. As the bulk of the cases are machine generated ( Based on the discussion, I certainly see the appeal of detection of three-body reactions, etc.. On the other hand, a self-documenting approach (enforce |
I don't see why a warning would be required -- the YAML format is not in any stable release, so I think we can make breaking changes as needed.
For the vast majority of user input, we rely on the user to write what they mean, and have no way of checking that that input is what they meant. Obviously, in those cases where we can detect inconsistencies, we should, but I don't think we should try to design the input format to require redundant data just so that we can check for consistency.
I think this is a simple and logically-consistent option that won't result in too much clutter in input files, while providing the opportunity to simplify reaction parsing. For now, I think this means that the only necessary change to the YAML specification is that electrochemical reactions need to be labeled as |
Perfect! |
Closing per discussion in #750: automatic detection is preferred over simplicity of parsing. Long-term, it would make sense to avoid duplicate parsing of surface reactions by other means. |
System information
Expected behavior
YAML entries clearly identify type of reaction, with the default being
elementary
. This is being followed consistently forGasKinetics
(withthree-body
, etc.), and the same should be done forInterfaceKinetics
etc., e.g.Edit: the type
interface
as default forInterfaceKinetics
does makes sense (similar toelementary
as default forGasKinetics
). The following is less intuitive, as thebeta
andexchange-current-density-formulation
fields are not required.Actual behavior
The
type
field is not used forinterface
andelectrochemical
reactions, which can only be detected by parsing the species string (the type of reaction is purely implicit). Further, several electrochemical reaction types appear to be discontinued in the YAML format without a deprecation warning.Additional context
The implicit specification of types is not future-proof, as it is not easily extendable. As YAML is not yet officially released, these clarifications can be easily added to 2.5.
The text was updated successfully, but these errors were encountered: