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.

Sign up for GitHub

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

Jump to bottom

Feature/ldf isotherms #104

Merged
merged 48 commits into from Dec 16, 2022
Merged

Feature/ldf isotherms #104

merged 48 commits into from Dec 16, 2022

Conversation

jazib-hassan-juelich
Copy link
Contributor

@jazib-hassan-juelich jazib-hassan-juelich commented Sep 20, 2021 •
edited by schmoelder

This branch contains LDF based implementations of Freundlich, Langmuir and Bi-Langmuir isotherms.

@jazib-hassan-juelich
Copy link
Contributor Author

jazib-hassan-juelich commented Nov 3, 2021 •
edited

Work update:

Implementation:

  • Freundlich Isotherm
  • Bi-Langmuir LDF Isotherm
  • Langmuir LDF Isotherm

Documentation:

  • Freundlich Isotherm
  • Bi-Langmuir LDF Isotherm
  • Langmuir LDF Isotherm

@schmoelder schmoelder force-pushed the feature/LDF_Isotherms branch 2 times, most recently from 515cfb7 to 6f3e457 Compare January 31, 2022 13:15
@schmoelder
Copy link
Contributor

I did run some tests to check the plausibility of the Langmuir and Bi-Langmuir LDF models by comparing them to the native approach and so far it looks all good! 🥳
Will run some more in the next couple of days. But hopefully we can close this PR soon! :)

@jazib-hassan-juelich
Copy link
Contributor Author

I did run some tests to check the plausibility of the Langmuir and Bi-Langmuir LDF models by comparing them to the native approach and so far it looks all good! 🥳 Will run some more in the next couple of days. But hopefully we can close this PR soon! :)

Great. I hope the next results also come fine

sleweke-bayer
sleweke-bayer suggested changes Mar 30, 2022
View reviewed changes
Copy link
Contributor

@sleweke-bayer sleweke-bayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tests for the Freundlich isotherm.

For binding model implementation, tests are really helpful. For example, they check your analytic Jacobian. If you had done this for the Freundlich model, you would've noticed that they fail.

src/libcadet/model/binding/FreundlichLDFBinding.cpp Outdated
Comment on lines 117 to 121
const double n_param = (double)static_cast<ParamType>(p->n[i]);
const double kF = (double)static_cast<ParamType>(p->kF[i]);
const double kkin = (double)static_cast<ParamType>(p->kkin[i]);
double const alpha_1 = ((2 * n_param - 1) / n_param) * kF * std::pow(threshold, (1 - n_param) / n_param);
double const alpha_2 = ((1 - n_param) / n_param) * kF * std::pow(threshold, (1 - 2 * n_param) / n_param);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not convert the active types to double! This will disable parameter sensitivities.
If you need to copy these to local variables, please do

const ParamType& n_param = static_cast<ParamType>(p->n[i]);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

src/libcadet/model/binding/FreundlichLDFBinding.cpp Outdated
Comment on lines 120 to 121
double const alpha_1 = ((2 * n_param - 1) / n_param) * kF * std::pow(threshold, (1 - n_param) / n_param);
double const alpha_2 = ((1 - n_param) / n_param) * kF * std::pow(threshold, (1 - 2 * n_param) / n_param);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use plain pow instead of std::pow. The latter will fail for active types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

src/libcadet/model/binding/FreundlichLDFBinding.cpp Outdated
Comment on lines 120 to 121
double const alpha_1 = ((2 * n_param - 1) / n_param) * kF * std::pow(threshold, (1 - n_param) / n_param);
double const alpha_2 = ((1 - n_param) / n_param) * kF * std::pow(threshold, (1 - 2 * n_param) / n_param);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use plain pow instead of std::pow. The latter will fail for active types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

src/libcadet/model/binding/FreundlichLDFBinding.cpp Outdated
Comment on lines 126 to 133
if (std::abs((double)yCp[i]) < threshold)
res[bndIdx] = kkin * y[bndIdx] - alpha_1 * yCp[i] - alpha_2 * std::pow((double)yCp[i], 2);
else
res[bndIdx] = kkin * y[bndIdx] - kkin * kF * std::pow(std::abs((double)yCp[i]), 1.0 / n_param);
}
else
{
res[bndIdx] = kkin * y[bndIdx] - kkin * kF * std::pow(std::abs((double)yCp[i]), 1.0 / n_param);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here: No std::pow or std::abs, use plain pow and abs.
Also: Do not convert to double.

Sidenote: Why not collect kkin into a common factor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done and put kkin into a common factor.

src/libcadet/model/binding/FreundlichLDFBinding.cpp Outdated
Comment on lines 126 to 133
if (std::abs((double)yCp[i]) < threshold)
res[bndIdx] = kkin * y[bndIdx] - alpha_1 * yCp[i] - alpha_2 * std::pow((double)yCp[i], 2);
else
res[bndIdx] = kkin * y[bndIdx] - kkin * kF * std::pow(std::abs((double)yCp[i]), 1.0 / n_param);
}
else
{
res[bndIdx] = kkin * y[bndIdx] - kkin * kF * std::pow(std::abs((double)yCp[i]), 1.0 / n_param);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here: No std::pow or std::abs, use plain pow and abs.
Also: Do not convert to double.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I use pow and don't convert it to (double) then I get type error.

src/libcadet/CMakeLists.txt
Comment on lines -237 to +242
add_library(libcadet_object OBJECT ${LIBCADET_SOURCES})
add_library(libcadet_object OBJECT ${LIBCADET_SOURCES} "model/binding/LangmuirLDFCBinding.cpp")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert

src/libcadet/CMakeLists.txt Outdated
Comment on lines 283 to 287
add_library(libcadet_nonlinalg_mex STATIC ${LIBCADET_NONLINALG_SOURCES} ${LIBCADET_NONLINALG_MEX_SOURCES})
add_library(libcadet_nonlinalg_mex STATIC ${LIBCADET_NONLINALG_SOURCES} ${LIBCADET_NONLINALG_MEX_SOURCES} "model/binding/LangmuirLDFCBinding.cpp")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert

src/libcadet/CMakeLists.txt Outdated
Comment on lines 283 to 287
add_library(libcadet_nonlinalg_mex STATIC ${LIBCADET_NONLINALG_SOURCES} ${LIBCADET_NONLINALG_MEX_SOURCES})
add_library(libcadet_nonlinalg_mex STATIC ${LIBCADET_NONLINALG_SOURCES} ${LIBCADET_NONLINALG_MEX_SOURCES} "model/binding/LangmuirLDFCBinding.cpp")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert

src/libcadet/CMakeLists.txt Outdated
Comment on lines 312 to 316
add_library(libcadet_mex STATIC ${LIBCADET_SOURCES})
add_library(libcadet_mex STATIC ${LIBCADET_SOURCES} "model/binding/LangmuirLDFCBinding.cpp")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert

src/libcadet/CMakeLists.txt Outdated
Comment on lines 312 to 316
add_library(libcadet_mex STATIC ${LIBCADET_SOURCES})
add_library(libcadet_mex STATIC ${LIBCADET_SOURCES} "model/binding/LangmuirLDFCBinding.cpp")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert

@schmoelder
Copy link
Contributor

Do not merge yet! Because this branch was not properly rebased on master in the past, there are some unwanted changes, especially in the documentation.

@sleweke
Copy link
Member

sleweke commented Oct 19, 2022

I've fixed the tests, the Jacobians, and (in some cases) the model equations. The tests now pass, the Jacobians should be correct.
Please let me know when the documentation is done. I'll clean up the commits a little and rebase this branch onto master.

@jazib-hassan-juelich
Copy link
Contributor Author

I've fixed the tests, the Jacobians, and (in some cases) the model equations. The tests now pass, the Jacobians should be correct. Please let me know when the documentation is done. I'll clean up the commits a little and rebase this branch onto master.

I have tested the implememtation on some known cases and its working and I will discuss the documentation with @schmoelder and try to finalize it in next one or two days.

schmoelder and others added 27 commits December 16, 2022 10:44
@schmoelder
Revert "Modified the Readme.md file"
57ecaff 
This reverts commit efba48e.
@jazib-hassan-juelich @schmoelder
Modified the Readme file based on @schmoelder review
5f92e1a
@jazib-hassan-juelich @schmoelder
Modified the Readme file based on @johannes review
d86c1e2
@jazib-hassan-juelich @schmoelder
Modified the interface parameters names
b145ce8
@jazib-hassan-juelich @schmoelder
Modified the documentation of the LDF isotherms
dd6a27c
@jazib-hassan-juelich @schmoelder
Modified the test of the LDF isotherms
6194c9d
@jazib-hassan-juelich @schmoelder
Modified Bi Langmuir isotherms documentation to include info about st…
83a7c4d 
…ate major ordering and correcting the required size of the parameters
@jazib-hassan-juelich @schmoelder
Modified the error message related to the consistency in defining bou…
dc72de7 
…nd states for each component in Bi-Langmuir models
@schmoelder
Revert changes to README
f4af6a0
@schmoelder
Fix header level
468bf97
@schmoelder
Add some more information to the model
0bdf1d9
@schmoelder
Formatting
173adc4
@schmoelder
Clarify ordering of multi dimensional data
36db06d
@jazib-hassan-juelich @schmoelder
Adding Langmuir LDF liquid phase model configuration in binding model…
c86107e 
… factory
@jazib-hassan-juelich @schmoelder
Adding Langmuir LDF liquid phase model source file
d8bf043
@jazib-hassan-juelich @schmoelder
Modified the calling string of Langmuir LDF Liquid phase model
95165a5
@jazib-hassan-juelich @schmoelder
Added documentation for Langmuir LDF liquid phase
f036cb6
@schmoelder
Added tests for Langmuir LDF liquid phase model.
bad785c
@schmoelder
Update documentation
f0c7583
@jazib-hassan-juelich @schmoelder
Removed the un-necessary lines from the CMakeSettings file
89550ec
@jazib-hassan-juelich @schmoelder
Made changes in the source code of Fruendlich isotherms based on the …
58c3580 
…review
@jazib-hassan-juelich @schmoelder
Added test for the Fruendlich isotherm
bbab2ec
@jazib-hassan-juelich @schmoelder
Modified jacimpl code according to review
8fb0c66
@jazib-hassan-juelich @schmoelder
Modified FLDF parameters in test script
4174f81
@schmoelder
Do not cast to double
0f72404
@schmoelder
Fix Test Parameters.
9f5dca9
@sleweke @schmoelder
Fix Jacobians and equations for LDF binding models
160b4f2 
Fixes several problems with the model equations and Jacobians of the LDF
binding models. Also fixes the tests, which all pass now.
@schmoelder schmoelder merged commit bee06f6 into master Dec 16, 2022
@schmoelder schmoelder deleted the feature/LDF_Isotherms branch December 17, 2022 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schmoelder schmoelder schmoelder left review comments

@sleweke-bayer sleweke-bayer sleweke-bayer requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jazib-hassan-juelich @schmoelder @sleweke @sleweke-bayer