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

[SIRIUS] libvdwxc functionality #2270

Merged
merged 4 commits into from
Sep 6, 2022
Merged

Conversation

mtaillefumier
Copy link
Contributor

Add an extra option to support libvdwxc/sirius functionality in cp2k.

@@ -53,6 +54,13 @@ MODULE input_cp2k_pwdft

PUBLIC :: create_pwdft_section

#if defined(__LIBVDWXC)
Copy link
Member

Choose a reason for hiding this comment

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

I think, these constants should be defined regardless of whether libvdwxc is present. We currently don't have a CI test for this, but I believe right now one can not build with SIRIUS and without libwdvxc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is possible to compile sirius without libvdwxc even with the toolchain. The reason is it is not a hard dependency so it is not enforced.

Copy link
Member

Choose a reason for hiding this comment

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

Right, the problem is that you are importing these constants in sirius_interface.F regardless of whether libvdwxc is present or not. The simplest solution would be to have them always defined.

@oschuett
Copy link
Member

oschuett commented Sep 6, 2022

LGTM.
Do you want to add a test for the new feature? It would require a new TEST_DIR conditioned on the libvdwxc flag.
Otherwise, I can also merge this PR as is.

@mtaillefumier
Copy link
Contributor Author

ideally we need tests cases for this but I prefer to see this validated with a real calculation than just a dummy test showing convergence (Opening an issue will remind me to do it).
One user had trouble with vdw that what triggered this PR. So it is probably better to merge the PR so he can use vdw with SIRIUS and at the same time helps validating it.

I ran his input file and SIRIUS converged without any problem . Of course gold is easy.

@oschuett oschuett merged commit eb97e8d into cp2k:master Sep 6, 2022
@mtaillefumier
Copy link
Contributor Author

thanks. I can now ask the user to give it a spin.

@mtaillefumier mtaillefumier deleted the sirius_vdw branch January 25, 2023 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants