adcc icon indicating copy to clipboard operation
adcc copied to clipboard

Nuclear Gradients

Open maxscheurer opened this issue 5 years ago • 36 comments

Implemented together with @Drrehn.

Features

  • [x] MP2
  • [x] ADC(0)
  • [x] ADC(1)
  • [x] ADC(2)
  • [x] ADC(2)-x
  • [x] ADC(3)
  • [x] CVS-ADC(1) (by @iubr)
  • [x] CVS-ADC(2) (by @iubr)
  • [x] CVS-ADC(2)-x (by @iubr)

ToDo

  • [ ] consolidate some of the CVS equations for different methods
  • [ ] Code documentation (docstrings etc.)
  • [ ] Warning about performance
  • [ ] Basic documentation (calculations.rst)
  • [x] merge on master (#189)
  • [x] rebase on #158
  • [x] finite difference data test generation (preliminary version working)
  • [x] tests:
    • [x] individual components (densities etc.)
    • [x] reference tests (preliminary version working)
  • [x] rebase once #123 is in (I've added some stuff from this branch here for testing purposes, will be removed)
  • [x] ~add equation numbers from papers (Rehn 2019, Levchenko 2005)~

maxscheurer avatar Jan 13 '21 16:01 maxscheurer

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

new alerts:

  • 1 for Unused local variable

lgtm-com[bot] avatar Jan 13 '21 16:01 lgtm-com[bot]

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

new alerts:

  • 1 for Unused local variable

lgtm-com[bot] avatar Jan 13 '21 17:01 lgtm-com[bot]

Awesome :+1:

mfherbst avatar Jan 14 '21 10:01 mfherbst

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

new alerts:

  • 1 for Unused local variable

lgtm-com[bot] avatar Jan 16 '21 12:01 lgtm-com[bot]

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

new alerts:

  • 1 for Unused local variable

lgtm-com[bot] avatar Jan 16 '21 12:01 lgtm-com[bot]

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

new alerts:

  • 1 for Unused local variable

lgtm-com[bot] avatar Jan 17 '21 10:01 lgtm-com[bot]

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

new alerts:

  • 1 for Unused local variable

lgtm-com[bot] avatar Jan 19 '21 10:01 lgtm-com[bot]

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

new alerts:

  • 1 for Unused local variable

lgtm-com[bot] avatar Feb 21 '21 09:02 lgtm-com[bot]

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

new alerts:

  • 1 for Unused local variable

lgtm-com[bot] avatar Jun 03 '21 13:06 lgtm-com[bot]

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

new alerts:

  • 3 for Unused import
  • 1 for Unused local variable

lgtm-com[bot] avatar Dec 14 '21 15:12 lgtm-com[bot]

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

new alerts:

  • 3 for Unused import
  • 1 for Unused local variable

lgtm-com[bot] avatar Dec 14 '21 18:12 lgtm-com[bot]

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?

mfherbst avatar Dec 15 '21 08:12 mfherbst

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

maxscheurer avatar Dec 15 '21 11:12 maxscheurer

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)

iubr avatar Dec 15 '21 12:12 iubr

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

maxscheurer avatar Dec 15 '21 12:12 maxscheurer

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.)

mfherbst avatar Dec 15 '21 12:12 mfherbst

You mean the convergence tolerance/atol for asserts?

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

maxscheurer avatar Dec 15 '21 12:12 maxscheurer

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.

mfherbst avatar Dec 15 '21 12:12 mfherbst

@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.

maxscheurer avatar Dec 17 '21 17:12 maxscheurer

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.

iubr avatar Dec 18 '21 13:12 iubr

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.

maxscheurer avatar Dec 18 '21 14:12 maxscheurer

Thanks a lot!

iubr avatar Dec 19 '21 00:12 iubr

Super cool! I'll review this weekend.

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

mfherbst avatar Feb 10 '22 06:02 mfherbst

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.

maxscheurer avatar Feb 11 '22 08:02 maxscheurer

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

iubr avatar Feb 11 '22 09:02 iubr

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.

maxscheurer avatar Feb 11 '22 09:02 maxscheurer

Thanks! I'll look into it.

iubr avatar Feb 11 '22 11:02 iubr

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).

iubr avatar Feb 13 '22 09:02 iubr

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

maxscheurer avatar Feb 13 '22 09:02 maxscheurer

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 avatar Feb 13 '22 23:02 iubr