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

Freundlich LDF differes between documentation and implementation. #357

Closed
ronald-jaepel opened this issue Jan 15, 2025 · 1 comment · Fixed by #260
Closed

Freundlich LDF differes between documentation and implementation. #357

ronald-jaepel opened this issue Jan 15, 2025 · 1 comment · Fixed by #260

Comments

@ronald-jaepel
Copy link
Collaborator

ronald-jaepel commented Jan 15, 2025

The Freundlich isotherm is documented as:

$q^\ast_i= k_{F,i}c_{p,i}^{1/n_{i}}$

but is implemented as

$q^\ast_i= k_{F,i}|c_{p,i}|^{1/n_{i}}$

The implementation change of taking the absolute value of $c_p$ is to prevent negative concentrations from crashing the simulation. This can happen if $1/n$ is chosen such that it leads to an even root being taken of $c_p$, e.g. $n=2$.

The linearization that we have implemented for small concentrations (to stabilize the derivative for $c_p \approx 0$) could also be used to stabilize the negative concentrations.

Should we update the documentation to match the implementation or update the implementation to use the linearization for small and negative $c_p$ values?

@lieres what do you think?

@lieres
Copy link
Member

lieres commented Jan 15, 2025

The linearization is generally more physical and should be preferred over taking the absolute value.

Then, the documentation should follow the code.

@ronald-jaepel ronald-jaepel linked a pull request Jan 15, 2025 that will close this issue
7 tasks
@github-project-automation github-project-automation bot moved this from Todo to Done in CADET-Project Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants