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

Analytical derivatives of the MO coefficients wrt nuclear coordinates and the evaluation of Atomic Polar Tensors #1706

Merged
merged 3 commits into from
Nov 17, 2021

Conversation

edditler
Copy link
Contributor

@edditler edditler commented Oct 28, 2021

Dear CP2K developers,

We used the code in this pull request for the calculations in this publication: doi.org/10.1063/5.0041056.
We added another response calculation called dcdr_linres to qs_linres_module. The purpose is the analytical evaluation of the derivatives of the MO coefficients with respect to nuclear coordinates. The routine dcdr_response_dR calculates $dCR = dC^0/dR^lambda_beta$, where lambda enumerates the atom indices and beta the cartesian coordinates. The coefficient derivatives are then used to calculate the Atomic Polar Tensors (APTs) $P^lambda_alpha,beta = d<mu_alpha>/dR^lambda_beta$, where mu_alpha are the components of the electric dipole operator. The routine apt_dR_localization calculates the APTs using the localized dipole operator and localized orbitals.

This pull request touches quite a few files in the core of CP2K to evaluate integral derivatives with respect to nuclear coordinates - instead of electronic coordinates. I hope that I didn't incur significant performance issues in the standard ground-state calculations.

This input: https://gist.github.com/edditler/97d8a3d2b8f23eb2bb0bcbee3bacfcfc results in this output: https://gist.github.com/edditler/0b517b67bd01346f5c6ad7674c5dc21c on my machine, using the sdbg, ssmp, and psmp binaries.

I ran the regression tests using the arch files generated by the toolchain with the command tools/regtesting/do_regtest -arch local -version psmp -maxtasks 10 -mpiranks 2 -ompthreads 2

One can also use the implementation to evaluate the analytical Hessian matrix and improve geometry optimizations, but we didn't consider the needed interface.

Best regards,

Edward

Copy link
Member

@oschuett oschuett left a comment

Choose a reason for hiding this comment

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

Thanks for contributing Edward!

The changes to the grid code look good to me.

Please consider adding a few regtests.

src/grid/grid_api.F Outdated Show resolved Hide resolved
src/grid/ref/grid_ref_integrate.c Show resolved Hide resolved
@edditler
Copy link
Contributor Author

edditler commented Oct 28, 2021

Regarding the regression tests: Do I understand right that I need to move the output from a separate file

cp2k/src/qs_dcdr_utils.F

Lines 707 to 709 in c623102

dcdr_env%output_unit = cp_print_key_unit_nr(logger, dcdr_section, "PRINT%APT", &
extension=".data", middle_name="dcdr", log_filename=.FALSE., &
file_position="REWIND", file_status="REPLACE")
into the main CP2K output to be able to compare the result of the sum rules/APTs in the regtests?

@mkrack
Copy link
Member

mkrack commented Oct 28, 2021

Yes. You need only to print a key result in the CP2K output file which you want to check, preferentially tagged e.g. with APT| .

@oschuett
Copy link
Member

Since you are already using a printkey, it should be sufficient to set it to __STD_OUT__ in the input file.

@oschuett
Copy link
Member

Unfortunately, there is still a merge conflict because our formatting changed in the meantime (ENDIF -> END IF). I'd suggest you run make pretty again and then squash all your changes into a single commit.

@edditler
Copy link
Contributor Author

Unfortunately, there is still a merge conflict because our formatting changed in the meantime (ENDIF -> END IF). I'd suggest you run make pretty again and then squash all your changes into a single commit.

This morning, I ran make pretty and observed that the number of merge conflicts increased. The force push reversed that commit. I wanted to avoid resolving merge conflicts too often. Is this PR ready for merging otherwise? I'll get on it first thing tomorrow.

Copy link
Member

@oschuett oschuett left a comment

Choose a reason for hiding this comment

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

I found two nits, otherwise I think this is ready to be merged.

src/qs_dcdr_utils.F Outdated Show resolved Hide resolved
src/qs_dcdr_utils.F Outdated Show resolved Hide resolved
@oschuett
Copy link
Member

I'm going to squash merge this, but I feel that "Response for dC/dR_lambda^beta" might not be a very useful commit message.
How about "Add analytical derivatives of MO coefficients wrt nuclear coordinates" ?

@edditler
Copy link
Contributor Author

Seems good! Can you change the commit message or should I push once more?

@oschuett oschuett merged commit 67208ce into cp2k:master Nov 17, 2021
@oschuett
Copy link
Member

It's done. Thanks for upstreaming your changes!

@oschuett
Copy link
Member

The dashboard found a couple of convention issues and what appears to be a double free.

Instead of cp_fm_gemm please use cp_gemm from cp_gemm_interface.F.

@edditler
Copy link
Contributor Author

Should I create a new PR for the fixes?

@oschuett
Copy link
Member

Yes, it would be great if you could fix these issue with a follow up PR.

@edditler edditler mentioned this pull request Nov 18, 2021
@mkrack
Copy link
Member

mkrack commented Nov 18, 2021

There are also issues with the tests using the Intel compiler.

@edditler
Copy link
Contributor Author

edditler commented Nov 18, 2021

There are also issues with the tests using the Intel compiler.

The assertion fails in https://github.com/cp2k/cp2k/blob/master/src/qs_dcdr_ao.F#L769-L771. I'm not sure what the fix is. Maybe replacing by CPASSERT(ASSOCIATED(k_block))? Code identical to these three lines seems to be used at other places in the codebase, or will the issue be somewhere else?

Two more failing tests, where I don't understand what's going wrong, are

@mkrack
Copy link
Member

mkrack commented Nov 18, 2021

The i386 test was failing already before your PR. The minimal, GCC7, and the Intel tests might have failed for the same reason, which is possibly fixed by your latest fix. We will see.

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