-
Notifications
You must be signed in to change notification settings - Fork 27
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
Add organic modifier LangmuirLDF binding #171
base: master
Are you sure you want to change the base?
Conversation
Hi and thanks for your PR. For consistency, could you please revert the changes to the parameter names? I know it's superfluous, but currently we would like to stick to the convention of a parameter prefix (i.e. Also, it might be a good idea to add some tests s.t. we can be more confident with the implementation. If you need help, please feel free to reach out or to visit on of our upcoming Office Hours. |
Dear Johannes,
Thank you for your email. Sorry for the late reply, I was out of office
until today. I will revise the code and I will let you know.
Best,
Kostas
…On Sun, 24 Mar 2024, 08:42 Johannes Schmölder, ***@***.***> wrote:
Hi and thanks for your PR. For consistency, could you please revert the
changes to the parameter names? I know it's superfluous, but currently we
would like to stick to the convention of a parameter prefix (i.e. MCLLDF_
).
Also, it might be a good idea to add some tests s.t. we can be more
confident with the implementation. If you need help, please feel free to
reach out or to visit on of our upcoming Office Hours
<https://forum.cadet-web.de/t/introducing-office-hours/518>.
—
Reply to this email directly, view it on GitHub
<#171 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/BEAHZOR4YTOMCS5PKCEL4ITYZ2GYTAVCNFSM6AAAAABDXDMEEGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJWG4ZTOMRQGQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Hi I committed the changes to the code. After a few tests I've done on Python, it seems to run smoothly. |
Hi, thanks for your input. Could you please also add tests and documentation to your PR? |
Hi @schmoelder, how can I upload tests and what kind of tests are considered adequate? |
Hi @Kats1247, sorry for the delay :) and thank you again for your contribution to the CADET project. TestsCreating teststhe tests are part of the cadet code. I''ll turn this comment into a guide in the future, but for now, here's a comment: The binding model tests are in
The syntax is defined in the header file
basically, you'll need to change the number of bound states (depending on if the mobile phase organic modifier can bind), and add the some values for the extra parameters you've added to the model. These just need to be some "reasonable" values that produce sensible results when used with your isotherm. Running testsIf you want to run tests in CADET-Core you need to ensure that in the
Then you can find the testRunner(.exe) in If you run into problems with that feel free to ask again or join us at the monthly Office Hours. DocumentationAll binding models need documentation. These are stored in the Thank you. :) |
A slightly transformed equilibrium approach to the Langmuir isotherm that accounts for gradient conditions. When disabling kinetics IS_KINETIC = 0, kkin has still an impact on the output.
Edit: This PR is a follow-up to a discussion in our Forum.