search

adc-connect / adcc

adcc: Seamlessly connect your program to ADC
https://adc-connect.org
GNU General Public License v3.0
32 stars 19 forks source link

Nuclear Gradients #124

Open maxscheurer opened 3 years ago

maxscheurer commented 3 years ago

Implemented together with @Drrehn.

Features

ToDo

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 6b92a096a84b5f4f49695d27b8d894a406e33ad9 into 6e7ac343b884610932033d9f6c340829ca72db95 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 7136d0362be005ed28033816bcf476a3228bd555 into 6e7ac343b884610932033d9f6c340829ca72db95 - view on LGTM.com

new alerts:

mfherbst commented 3 years ago

Awesome :+1:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 844cc1c2c65002156f6dac589710401a5e01df48 into 6e7ac343b884610932033d9f6c340829ca72db95 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 10980bf2da4a894772755538443d76be756f7d69 into 6e7ac343b884610932033d9f6c340829ca72db95 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 3f826183fad9d611a78eb15b9ff14c4bf34e54ce into 6e7ac343b884610932033d9f6c340829ca72db95 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging b92ee1b6592e6176124fbbbc7b25356eae4e662e into 041cdcb16ec60d1655db8d562ef2f3b53038c120 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 1a3c802c3068a4c6e33d8304923212fe5c2d6f2c into 041cdcb16ec60d1655db8d562ef2f3b53038c120 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 54a9181092d280331a75c1d48e976aeda5f2bdb3 into e4ca2edf77884474bb685122efc272a900151509 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 2 years ago

This pull request introduces 4 alerts when merging 92e753579dcf26266eb48decb8862567fe817876 into e4ca2edf77884474bb685122efc272a900151509 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 2 years ago

This pull request introduces 4 alerts when merging df675fa4308f02b62aa78863c40c4ac8176d0b31 into e4ca2edf77884474bb685122efc272a900151509 - view on LGTM.com

new alerts:

mfherbst commented 2 years ago

Of course tests need to work and all, but I wonder if a good first aim is to just get the things already implemented merged into master and release a new version?

maxscheurer commented 2 years ago

The tests are horrible to implement... ensuring highly converged eigenvectors is difficult 😞 but in principle all the implemented gradient methods are correct 👍🏻

iubr commented 2 years ago

Not sure if this is a good suggestion, but maybe we could run a couple of small molecules and save the results in a text file for pytest to compare agains (this is more or less how it's done in veloxchem). I can try out the cvs-adc gradients and find an optimal/good enough setup (I guess the question is of comparing analytical vs. numerical gradients)

maxscheurer commented 2 years ago

The generation of finite-difference reference data works fine already, for both generic and CVS-ADC 😊👍🏻

mfherbst commented 2 years ago

The tests are horrible to implement... ensuring highly converged eigenvectors is difficult

Can't you lower to tolerance or introduce perturbations in the molecule / orbitals to ensure they are sufficiently distinct (to avoid rotations in invariant subspaces etc.)

maxscheurer commented 2 years ago

You mean the convergence tolerance/atol for asserts?

You mean geometry perturbations to break symmetries? Yes, I can try that for sure 👍🏻

mfherbst commented 2 years ago

You mean the convergence tolerance/atol for asserts?

Yes. It's not amazing, but it will at least catch forgotten factors of two or switched signs.

maxscheurer commented 2 years ago

@iubr, the CVS-ADC2-x don't seem to be correct right now, at least I get an error for H2O of ~1e-5 (compared to 5-point finite differences). Maybe I made some mistake while cleaning up the code... Which accuracy could you obtain in your tests? I'll try to run your original branch again at some point to get to the bottom of this.

iubr commented 2 years ago

The ~1e-5 is similar to what I get with my code (in comparison to the 2-point finite difference numerical gradient). I checked it today with a 5-point finite difference and it is a little smaller, but still same order (1e-5). So if there is a bug, it would be in my module as well. I had discussed this difference with Patrick and we had concluded that it could be due to the tolerance/convergence threshold for the adc step (I couldn't go better than 1e-6 because of the requirement for the HF reference). Of course, this might not be the right explanation for the difference. It's, of course, possible that there is still an undiscovered bug or mistake somewhere in the code/equations. I'll make another check next week. Let's see.

maxscheurer commented 2 years ago

I had discussed this difference with Patrick and we had concluded that it could be due to the tolerance/convergence threshold for the adc step

I converge the SCF to 1e-12, and the ADC procedure is converged to 1e-10 in the energy. Given the fact that all other gradient methods so far can achieve at least 1e-7 accuracy, I'm convinced that one can achieve the same for CVS-ADC2-x . I'll try to add canonical ADC2-x gradients now and see how accurate one can get.

iubr commented 2 years ago

Thanks a lot!

mfherbst commented 2 years ago

Super cool! I'll review this weekend.

How do we compare speed-wise to alternative ADC(3) gradient implementations?

maxscheurer commented 2 years ago

There's no point in comparing our implementation to something else, because the bottleneck right now is the computation of ERI integral derivatives in the host programs for contraction with the TPDM. To make this more efficient, however, a lot of work would be required (as per comment in the code)... so I think it's best to add the gradients anyways but throw a warning that one should not expect the greatest performance 😄

A quick first review pass would be great, I think I need to do some cleanup anyways...

Any news on the CVS-ADC(2)-x gradients, @iubr? I found one error in the TPDM formation (now the correlation energy computed with the unrelaxed densities is at least correct), but I don't have time to investigate this further. If we cannot fix CVS-ADC(2)-x, I'd say we disable it for now, and merge the PR with the currently working features.

iubr commented 2 years ago

Hej Max,

Sorry for the long silence. There have been a couple of things that I had to prioritize, so I couldn't focus on cvs-adc2x, but I have been debugging these last two weeks. I didn't find anything yet, but still have some ideas of what to try (I confirmed the excitation energy PDMs are correct and tested the lambda multipliers against numerical dipole moments -- the error is the same as in cvs-adc2). I am now re-checking all the omega multipliers, but I am not sure how much longer it will take me to get to the bottom of it. I hope not too long. Let me see if I can do it over the weekend, otherwise please go ahead without cvs-adc2x. What was the error in the correlation energy TPDMs?

And thanks a lot for all the help! Super-exciting that you managed to add adc2x and adc3!!

/Iulia

maxscheurer commented 2 years ago

I fixed the TPDMs for CVS-ADC(2)-x in this commit, where the prefactor of g2a.vvvv was wrong.

The "correlation part" of the energy is tested here.

iubr commented 2 years ago

Thanks! I'll look into it.

iubr commented 2 years ago

I double checked the equations for the TPDMs and and I do get a factor 2 in the VVVV block for cvs-adc2x. In my module, it's the only way I get the correct excitation energy. I also wrote a stand-alone code to compute the excitation energy from the density matrices and without this factor 2 I do not get the correct energy. I attach the code here as a txt file in case you'd like to have a look cvs_adc2x_dms_from_scratch.txt . I still have to have a more careful look at the adcc version to understand what's going on, but next week is quite crazy for me up until Thursday. It could also be great to have a short meeting -- maybe you spot something I don't in the equations. What do you think?

Regarding cvs-adc2x updates, over the weekend I still haven't managed to figure out the reason why analytical vs. numerical agree only up to 1e-5 and not 1e-7 (as for the other adc orders). Maybe it's best you go ahead without cvs-adc2x for the moment and we add it once this mystery is solved (which I hope will not be that much longer).

maxscheurer commented 2 years ago

Thanks for the script, I'll have a look.

iubr commented 2 years ago

Thanks! If you'd like an overview of the density matrices, they are from https://doi.org/10.1063/5.0058221, Appendix C, Table II, 3rd column (ADC matrix terms).

iubr commented 2 years ago

Hi Max,

I just found the bug! It was a sign in one of the terms for the right-hand-side of the CV block, lambda multipliers. See here. I tried to merge the latest version of adcc/gradients branch into my cvs_gradients branch, but for some reason I cannot get it running on my computer, so I fixed the bug using the old version of the code. If you could make the fix at your end and test cvs-adc2x again, it would be super great!! Thanks a lot!! I'll make a few more tests on my end just to make sure there's nothing else.

/Iulia

maxscheurer commented 2 years ago

That's great news! I will try it out later. 👍🏻😊

iubr commented 2 years ago

Thank you!! Let me know how it goes :D (I still haven't figured out the puzzle of the factor 2 in the VVVV block between my module and adcc, but I'm planning to look into it again tomorrow)

maxscheurer commented 2 years ago

I found the reason for the weird factor 2 behavior in this block of code. The blocks oooo and vvvv were added again in the case of CVS, such that they existed twice in the TwoParticleDensityMatrix object. 😁

I found some other inconsistencies in the formation of g2a_hf and g2a_oresp, with these fixes altogether, everything works!!! 💪

mfherbst commented 2 years ago

Cool! Great work.

iubr commented 2 years ago

Wonderful ^-^!! Thanks a lot @maxscheurer!!