Change CSTR model to support variable porosity #245
Conversation
jbreue16
added
unit operation model
good first issue
As discussed yesterday, for downward compatibility, I would suggest exposing both, where |
|
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 UPDATE: |
jbreue16
changed the title
CMakeSettings.json
Outdated
There was a problem hiding this comment.
The changes in this file should not be added to master and we should also not add them to a PR branch so that we dont mess it up later.
| const double sVolume = paramProvider.exists("CONST_SOLID_VOLUME"); | ||
| const double initVolume = paramProvider.getDouble("INIT_VOLUME"); | ||
|
|
||
| if (sVolume != initVolume / eps - initVolume) |
There was a problem hiding this comment.
This is an exact floating point comparison. Due to round-off and / or representation errors (e.g., 0.3 cannot be represented exactly), it may not work. Allow some slack by introducing relative and / or absolute tolerance: https://codingnest.com/the-little-things-comparing-floating-point-numbers/
|
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
force-pushed
the
feature/CSTR_variable_porosity
branch
4 times, most recently
from
a57704d to
2b470fb
Compare
jbreue16
force-pushed
the
feature/CSTR_variable_porosity
branch
from
697064a to
438ca75
Compare
In the previous implementation, the CSTR supported variable total volume with constant porosity. Now, only the liquid volume is variable but the solid volume is fixed. Thus, the porosity is also variable. * Changed CSTR implementation * Adapted tests to repsoduce previous test cases * Updated documentation Co-authored-by: Antonia Berger <[email protected]>
jbreue16
force-pushed
the
feature/CSTR_variable_porosity
branch
from
438ca75 to
4920f2f
Compare
jbreue16
changed the title
Fixes #249 and #211
As per discussion in #211, we need to make porosity variable for the CSTR.
TODO
CSTRVarPorbased on theCSTRunit operation. This is described in the dev guide; please also leave a comment or make changes to the guide.POROSITYtoCONST_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.