-
Notifications
You must be signed in to change notification settings - Fork 37
Improved reactions interface #77
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
base: master
Are you sure you want to change the base?
Conversation
Thanks for the analysis and suggestion. I generally agree.
This suffix is not a convention (as of now). This fully depends on how the reaction model is implemented.
I don't see how this could work. Please elaborate some more. |
After some discussions, here is a proposal for a new reactions interface which would
|
aef551d
to
5d4ed30
Compare
While working on enabling multiple reactions, we developed a slightly different interface than the one proposed in the previous comment. Summary: Then, we specify each reaction model as follows:
Additionally, we now distinguish between intra-phase (within a single phase) and cross-phase reactions. For more details and concrete example, please refer to #92 and #453, #449. |
Fixes #67
Changes:
REACTION_MODEL_BULK
(previously alwaysREACTION_MODEL
)reaction_bulk
(previously mostlyreaction_bulk
, sometimesreaction
)The old version led to a lot of user errors; the newer version is more in more consistent and in in line with the particle reactions interface:
REACTION_MODEL_PARTICLES
reaction_particle
It does introduce some breaking changes in the interface but since reactions are quite new, there probably are actually very few people who use them. We could still read from the old location and throw a DeprecationWarning or, even better, this might also be a good opportunity to think again about how to realize #18.
Open question: Ambiguity in parameter naming
The proposed change still leaves some ambiguity in the naming of the parameters within the parameters branch.
From a discussion in the CADET Forum:
Thus, even though, we're defining a bulk reaction, we have to use the suffix
_liquid
for bulk reactions in that particular unit instead of_bulk
:More consistent would be:
However, this gets tricky as soon as solid phase reactions also have to be considered (which are part of the particles interface) and we still want to be able to make cross-phase reactions between liquid (in this case, bulk) phase, and solid phase...
Proposal:
Since the parameters themselves also have the suffices
_bulk
,_liquid
, and_solid
, we might simply unify the parameters reaction_model_bulk, and reaction_model_particle into onereaction_model
/reaction
field.This would require:
_bulk
parameters for the lumped rate model without pores (if we continue using the particle reactions interface internally)An interesting discussion with @lieres also led to the conclusion that in future we might want to provide even more flexibility with reactions (e.g. not only cross-phase but also cross-zone) which would also require modifying the interface in some way.