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

Radial flow for chromatography column models #120

Merged
merged 5 commits into from May 3, 2024
Merged

Conversation

sleweke
Copy link
Member

@sleweke sleweke commented Dec 9, 2022

Adds radial flow to chromatography column models. Only supports the upwind scheme as of now (i.e., no WENO) and uses constant dispersion coefficient.

The column model classes are now templates that take the type of the convection-dispersion-operator class. This required condensing the compilation down to one translation unit (TU) per column model.

For each column model, we add a second one with radial flow. That is, if UNIT_TYPE is GENERAL_RATE_MODEL, then using RADIAL_GENERAL_RATE_MODEL will give you the one with radial flow. For the geometry, the fields COL_RADIUS_INNER and COL_RADIUS_OUTER have been added. A positive velocity indicates flow from inner to outer radius.
To emphasize the difference of having a velocity field rather than a global velocity, we name the input field VELOCITY_COEFF, as opposed to VELOCITY which is used for the axial flow models

To do

  • Infrastructure for Parameter-Parameter Dependence
    • Base Class
    • Add flag to define whether absolute value should be used for dependency (e.g. for reverse flow, we do not want Dispersion to be negative)
    • create own PR with subsequent issues: add more parameter dependencies #189
  • Implement concrete implementations for Parameter-Parameter-Dependence: -> PowerLaw
  • Register PowerLaw Parameters
    • Dispersion
    • Film Diffusion
  • open issue: Check literature for reference solutions (documented in Fix and Expand tests part II #182 )
  • rebase. Old branch can be found here
  • Add unit/integration tests
  • Documentation once we have settled on the parameter names

@sleweke sleweke requested a review from lieres December 9, 2022 00:06
@sleweke sleweke added the feature Feature request label Dec 9, 2022
@schmoelder
Copy link
Contributor

Hey Sam,
thanks for all the work! We're going to send the files to our collaborators in India who will test it. Since they have domain knowledge, maybe they also have a good idea regarding correlations of the dispersion coefficient you mentioned in the forum (see here for reference).

@jbreue16, could you do us a favor and compile the branch for Windows?

@jbreue16
Copy link
Contributor

Hey Sam,
somehow M_PI is not defined in the ConvectionDispersionOperator.cpp. Accordingly, I (ifndef) defined _USE_MATH_DEFINES.
Furthermore, against your conventions, I had to move up the cmath include to the top. Ive checked the #include .hpp files and none of them used cmath without the _USE_MATH_DEFINES, so I dont really know why this had to be done.

Apart from this, the PR compiles and runs fine on my windows machine :)

@jbreue16
Copy link
Contributor

@schmoelder compiled for windows

cadet.zip

@schmoelder schmoelder force-pushed the feature/radial_flow branch 4 times, most recently from 65d5f91 to c18b15f Compare January 20, 2023 16:35
@lieres lieres requested review from jbreue16 and removed request for lieres February 27, 2023 16:24
@lieres
Copy link
Member

lieres commented Feb 27, 2023

Do we still want this in the main branch or shall we better wait for the DG version?

@schmoelder
Copy link
Contributor

schmoelder commented Mar 17, 2023

todo list moved to the upmost comment

@schmoelder
Copy link
Contributor

schmoelder commented Mar 22, 2023

In CADET, different parameter-parameter dependencies can be specified and the user is free to use any of the implemented dependencies for dependent parameters.
For example, the dispersion coefficient is usually assumed to be proportional to the velocity which can be modeled using a POWER_LAW dependency.

$$ p_{new} = \alpha p^k $$

where $k = 1$.

Note, in case of axial dispersion, the dependency is implemented the following way:

$$ D_{ax, dep} = D_{ax} \cdot \alpha \cdot u^k $$

To specify this, set

model.root.input.model.unit_001.col_dispersion = 1e-6
model.root.input.model.unit_001.col_dispersion_dep = 'POWER_LAW'
model.root.input.model.unit_001.col_dispersion_dep_base = 1
model.root.input.model.unit_001.col_dispersion_dep_exponent = 1

Moreover, a flag can be set which only uses the absolute value of the independent parameter.
This is useful here since flow can be reversed (i.e. a negative velocity) but this should not result in a negative dispersion coefficient.

model.root.input.model.unit_001.col_dispersion_dep_abs = 1

@schmoelder
Copy link
Contributor

as mentioned in #121, we need to add sudo apt update in the ci.

@schmoelder schmoelder force-pushed the feature/radial_flow branch 3 times, most recently from f2e4c68 to c2c8ce4 Compare April 29, 2023 05:37
@schmoelder
Copy link
Contributor

schmoelder commented May 9, 2023

@jbreue16 and I had another idea regarding the implementation of the dependencies.

Right now, e.g. in case of axial dispersion we have the following situation:

model.root.input.model.unit_001.col_dispersion = 1e-6
model.root.input.model.unit_001.col_dispersion_dep = 'POWER_LAW'
model.root.input.model.unit_001.col_dispersion_dep_base = 1
model.root.input.model.unit_001.col_dispersion_dep_exponent = 1

Here, a "base" value for D_ax is set which is then multiplied (this is hard-coded) with some configurable dependency on the velocity (which is also hard coded).

const ParamType d_ax_left = d_ax * p.parDep->getValue(p.model, ColumnPosition{relCoord, 0.0, 0.0}, comp, ParTypeIndep, BoundStateIndep, static_cast<ParamType>(p.u));

https://github.com/modsim/CADET/blob/c2c8ce4113adb9ecb27b9e67f7082691b24b2678/src/libcadet/model/parts/AxialConvectionDispersionKernel.hpp#LL141C22-L141C171

However, parameter dependencies might not always be multiplicative but could also be additive. For this purpose we also had to implement a CONSTANT_ONE type, as well as a CONSTANT_ZERO type.

Wouldn't it make sense to include the respective parameter in the arguments of getValue? This way, each implementation of the dependency type could "choose" whether it wants to add or multiple.

Maybe something like this:

const ParamType d_ax_left = p.parDep->getValue(d_ax, p.model, ColumnPosition{relCoord, 0.0, 0.0}, comp, ParTypeIndep, BoundStateIndep, static_cast<ParamType>(p.u));

At first glance, Jan and I couldn't see any downsides but maybe you have another thought, @sleweke?

@sleweke UPDATE (26.05.2023):
In the last commit [4a48aac] we did the above for parameter parameter dependencies. For parameter state dependencies we should probably stick to the old logic since the parameter values are assembled (e.g. for surface diffusion) from multiple calls of the parameter state dependency.

UPDATE (16.06.2023):
The above was confirmed. Next is the addition of parameter dependencies for the 2D GRM.

@jbreue16 jbreue16 force-pushed the feature/radial_flow branch 2 times, most recently from 2c63233 to 5c5c55c Compare April 19, 2024 18:34
@jbreue16 jbreue16 mentioned this pull request Apr 19, 2024
5 tasks
@jbreue16 jbreue16 mentioned this pull request Apr 30, 2024
34 tasks
@jbreue16 jbreue16 force-pushed the feature/radial_flow branch 2 times, most recently from a6a6727 to ebd87e2 Compare May 2, 2024 17:13
Co-authored-by: jbreue16 <jan.breuer@fz-juelich.de>
sleweke and others added 4 commits May 3, 2024 17:43
Spatial FV discretization units only

Co-authored-by: jbreue16 <jan.breuer@fz-juelich.de>
Adds infrastructure code (e.g., factories, interfaces) to allow
parameters to depend on other parameters.

Co-authored-by: jbreue16 <jan.breuer@fz-juelich.de>
Adds a parameter dependence that simply outputs the given parameter
value.
Add radial flow and parameter dependence support to createLWE

Axial dispersion for all FV units, film diffusion parameter dependence for LRMP FV unit

Add tests for radial flow convection dispersion operators

Enable radial dispersion coeff dependency in operators

Parameter dependencies are work in progress and the interfaces might change up until the next release

Co-authored-by: jbreue16 <jan.breuer@fz-juelich.de>
@jbreue16 jbreue16 merged commit d2b4c27 into master May 3, 2024
3 checks passed
@jbreue16 jbreue16 deleted the feature/radial_flow branch May 3, 2024 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants