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

Add a CSTR with variable porosity

Open AntoniaBerger opened this issue 1 year ago • 3 comments

Fixes #211 As per discussion in #211, we need to make porosity variable for the CSTR.

TODO

  • [x] Create new unit operation CSTRVarPor based on the CSTR unit operation. This is described in the dev guide; please also leave a comment or make changes to the guide.
  • [x] Adapt the interface: Change POROSITY to CONST_SOLID_VOLUME, which will be the constant solid volume of the tank. Porosity will be computed from that and not be exposed to the user anymore.
  • [x] Refactor the residual and Jacobian according to the new equations
  • [x] testing:
    • [x] new and old CSTR yield same result for constant volume simulations (test for simulations with/without binding, reaction)
    • [x] adjust CSTR tests

We will then have to decide on

  • [x] Should we keep the old CSTR? -> NO!
  • [x] If we dont keep the old CSTR, do we want a breaking change since this is a new, different model? Hide from documentation that its not actually a breaking chenge, i.e. change doc to new interface, but allow the old interface to still work, BUT give a warning if its used.
  • [x] Update documentation
  • [ ] Add test where CSTR runs empty wrt liquid volume
  • [ ] Fix tests: three tests are failing, one of them also failed before (last one in CSTR-Simulation)
  • [ ] fix consistent initialization (which is probably the reason for one of the failing tests)

AntoniaBerger avatar Jul 11 '24 11:07 AntoniaBerger

* [ ]  Adapt the interface: Change `POROSITY` to `CONST_SOLID_VOLUME`, which will be the constant solid volume of the tank. Porosity will be computed from that and not be exposed to the user anymore.

As discussed yesterday, for downward compatibility, I would suggest exposing both, where CONST_SOLID_VOLUME takes precedence. I.e. when only POROSITY is given, take that value and compute CONST_SOLID_VOLUME from it. If both are given, ignore POROSITY.

schmoelder avatar Jul 11 '24 11:07 schmoelder

Maybe we want a breaking change since this is a completely new unit/model? It might actually make sense to force users to acknowledge that the model has changed.

Anyways, we should probably first decide if we are going to keep the old CSTR, because if yes then the discussion is obsolete.

If we want to expose porosity again, it would probably also make more sense to call it INIT_POROSITY here.

UPDATE: We have decided to make changes to the interface but support the old interface (undocumented) and print warnings that the model has changed when the old interface is being used

jbreue16 avatar Jul 11 '24 12:07 jbreue16

Ive rebased, deleted the old CSTR and added backwards compatibility with the old interface with warnings.

I dont think we need the porosity as an output from CADET-Core since this is already given by the liquid volume .. that could be considered in a frontend like cadet-process or just leave it to the user.

The code still needs some clean up (e.g. change the descriptive comments to the new model) and maybe some changes for the consistent initialization.

jbreue16 avatar Aug 13 '24 10:08 jbreue16