CADET-Core
CADET-Core copied to clipboard
Add documentation for colloidal model
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
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?
I'm afraid not. I don't believe I've looked at the colloidal model at all.
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.
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 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.
Please remove the launch.vs.json file and changes in CMakeSettings.json before merging.
All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.
- [x] The CADET maintainers know my real name.
I have read the CLA Document and I hereby sign the CLA.
@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?
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.
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.
Looks good. Thank you everyone for your work!