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

Improved reactions interface #77

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

schmoelder
Copy link
Contributor

@schmoelder schmoelder commented May 18, 2021

Fixes #67

Changes:

  • Bulk reaction model is always read from REACTION_MODEL_BULK (previously always REACTION_MODEL)
  • Bulk reaction parameters are always read from reaction_bulk (previously mostly reaction_bulk, sometimes reaction)

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:

  • Particle reaction model is always read from REACTION_MODEL_PARTICLES
  • Particle reaction parameters are always read from 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:

Internally, CADET has some functions to handle a cell in a particle (i.e., radial shell consisting of liquid and solid phase). This function is also used for the lumped rate model without pores (the bulk volume cells also consist of liquid and solid phase).
Hence, you’ll have to use the interface for reactions in particles

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:

cadet.root.input.model.unit_001.reaction_model_bulk = 'MASS_ACTION_LAW'
cadet.root.input.model.unit_001.reaction_bulk.mal_kfwd_liquid = kfwd
cadet.root.input.model.unit_001.reaction_bulk.mal_kbwd_liquid = kbwd
cadet.root.input.model.unit_001.reaction_bulk.mal_stoichiometry_liquid = stoich

More consistent would be:

cadet.root.input.model.unit_001.reaction_model_bulk = 'MASS_ACTION_LAW'
cadet.root.input.model.unit_001.reaction_bulk.mal_kfwd_bulk = kfwd
cadet.root.input.model.unit_001.reaction_bulk.mal_kbwd_bulk = kbwd
cadet.root.input.model.unit_001.reaction_bulk.mal_stoichiometry_bulk = stoich

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...

cadet.root.input.model.unit_001.reaction_model_particles = 'MASS_ACTION_LAW'
cadet.root.input.model.unit_001.reaction_particles.mal_kfwd_solid = kfwd_solid
cadet.root.input.model.unit_001.reaction_particles.mal_kbwd_solid = kbwd_solid
cadet.root.input.model.unit_001.reaction_particles.mal_stoichiometry_solid = stoich_solid

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 one reaction_model/reaction field.

This would require:

  • Implementing an adapter to convert the _bulk parameters for the lumped rate model without pores (if we continue using the particle reactions interface internally)
  • Introducing a multiplex in case someone wanted to have have different reaction models (not parameters) for the different phases.
  • Optional: Introducing a flag to specify if model and parameters should be used for all (liquid) phases

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.

@schmoelder schmoelder changed the title modified reactions interface Improved reactions interface May 18, 2021
@schmoelder schmoelder marked this pull request as draft May 18, 2021 13:37
@sleweke
Copy link
Contributor

sleweke commented Jun 3, 2021

Thanks for the analysis and suggestion. I generally agree.

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 one reaction_model/reaction field.

This suffix is not a convention (as of now). This fully depends on how the reaction model is implemented.

Introducing a multiplex in case someone wanted to have have different reaction models (not parameters) for the different phases.

I don't see how this could work. Please elaborate some more.

@jbreue16 jbreue16 added this to the v6.0.0 milestone Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify reaction parameters interface
3 participants