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

Add documentation for colloidal model

Open schmoelder opened this issue 4 years ago • 4 comments
trafficstars

This PR included the documentation for the previously undocumented colloidal model by Xu et al, implemented for Vijesh Kumar.

Before merging, some reviewing still needs to be conducted:

  • [x] Provide some general information about the purpose and assumptions of the model.
  • [x] Add some more information about the coordination number. Also, I used n as the equation symbol (like here: https://en.wikipedia.org/wiki/Coordination_number)
  • [x] A bracket was missing in the model equation (after exp⁡(-κ~[R-(r_i+r_j )]∙(3+κR))). Check whether it was corrected appropriately.
  • [x] In the parameters section, K_kin is described as adsorption kinetics, but in the text as reaction kinetics. Check which one is correct.
  • [x] What is the meaning of the constants $\kappa _{c}$, $\kappa _{ef}$, and $\kappa _{f}$? Are they just fitting parameters?
  • [x] COL_KAPPA_FACT, COL_KAPPA_CONST don't use SI units. Is this hard coded into the model? Usually, we want to only expose 'real' SI units (i.e. meter in this case) to the user.
  • [ ] Fix Jacobian
  • [ ] Check tests

schmoelder avatar Jun 18 '21 14:06 schmoelder

From the documentation point of view, I think we're ready to merge. We should, however, consider whether we want to take the opportunity to check if we can fix the Jacobian. Also, there were some issues with testing (probably related to the Jacobian); @jayghoshter do you remember what that was?

schmoelder avatar Sep 23 '21 16:09 schmoelder

I'm afraid not. I don't believe I've looked at the colloidal model at all.

jayghoshter avatar Sep 24 '21 05:09 jayghoshter

Following up on my issue from Cadet-Process: https://github.com/fau-advanced-separations/CADET-Process/issues/14

Is this model usable? I see that some people are already using it within Cadet for affinity chromatography, which is what I'm interested in. https://forum.cadet-web.de/t/generic-load-wash-elution-strip-script/595. I'd like to use it with Cadet-Process if it's possible.

Is the lack of an analytic Jacobian a complete showstopper, or can it be estimated numerically? New to Cadet and Cadet-Process, so I'm not sure what the limitations are.

abe-martin avatar Feb 03 '23 17:02 abe-martin

The missing analytic Jacobian is not a complete showstopper. CADET can compute the entire system Jacobian by algorithmic differentiation (AD). This is however inefficient as we do have implemented analytic formulas for most Jacobian entries.

Providing analytic Jacobians for new code modules can be challenging. We are hence planning to provide a new option to selectively use AD and analytic formulas for different code modules.

lieres avatar Feb 06 '23 18:02 lieres

@lieres Quite some people have asked me about this isotherm and I believe, we should officially include it in CADET. I know its Jacobian is not yet implemented but maybe we could have a talk with Vijesh or other members of the Lenhoff lab if they could derive it. Obviously, having #122 in place would also make things easier because it would not slow down everything.

schmoelder avatar Jun 03 '24 16:06 schmoelder

Please remove the launch.vs.json file and changes in CMakeSettings.json before merging.

sleweke-bayer avatar Jun 26 '24 07:06 sleweke-bayer

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

github-actions[bot] avatar Jun 26 '24 10:06 github-actions[bot]

  • [x] The CADET maintainers know my real name.

schmoelder avatar Jun 26 '24 13:06 schmoelder

I have read the CLA Document and I hereby sign the CLA.

schmoelder avatar Jun 26 '24 13:06 schmoelder

@jbreue16, you approved but there are still 2 open To-Dos for the PR. Should we wait for them to be addressed or merge and create a follow-up issue/PR?

schmoelder avatar Jun 26 '24 13:06 schmoelder

I asked for Jans review too early, sorry. I think we need to decide on the naming disparity first before merging. The pH question also affects other pH dependent isotherms (like GIEX) so it could turn into it's own PR.

ronald-jaepel avatar Jun 26 '24 14:06 ronald-jaepel

Ive updated the todo by changing from "Unify names between model documentation and interface documentation. Currently the model documentation calls the parameters $k_a$ to $k_e$ while the interface uses COL_LOGKEQ_SALT_POWEXP, COL_LOGKEQ_SALT_POWFAC etc. which is confusing to new users." to "update interface documentation with the coefficient names from the equations" and added the according changes.

This way, we keep the more descriptive names in the interface but everyone can also easily trace back the input variables to the equations

I went through the whole documentation, fixed two things and I think this is now good to be merged.

jbreue16 avatar Jul 09 '24 18:07 jbreue16

Looks good. Thank you everyone for your work!

ronald-jaepel avatar Jul 10 '24 10:07 ronald-jaepel