CADET-Core icon indicating copy to clipboard operation
CADET-Core copied to clipboard

Improved reactions interface

Open schmoelder opened this issue 3 years ago • 1 comments

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 avatar May 18 '21 10:05 schmoelder

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.

sleweke avatar Jun 03 '21 19:06 sleweke