LR-PCM ADC for PSI4

[jonasleitner]

For some reason the PSI4 PE and PCM tests don't work when executed both. When executed separately they pass. Solved

[mfherbst]

Just did a quick pass over Max' comments. I'll do a more thorough read once you two have settled. Just ping me again.

[mfherbst]

But thanks for the contribution @jonasleitner! This actually solves part of the angle of a project I'm involved with, so thanks for doing some of my work ;).

[jonasleitner]

Moved the PCM HF calc to the test file... Using a dict atm, but not sure how flexible the PCM options need to be. (For now they could also be fixed) The Yaml generator works, but I don't like the layout of the lists. Would prefer the lists to be printed as lists. (As in the psi4_dump.yml)

[jonasleitner]

fixed the yaml generator. The documentation of the package is just awesome.

[maxscheurer]

Almost ready I think 👍 @mfherbst, can you give this another look if you have the time 😄 The py39 pipeline fails due to the newest h5py not being compatible with pyscf<2.0.0, but pyscf==2.0.0 is not yet on pypi... Coverage decreases because we don't test Psi4 in our CI pipeline. Maybe we should add a minimal conda build & test (only h2o_sto3g) with Psi4 in a follow-up PR to our CI?

[jonasleitner]

I added the ptLR correction for PSI4 to this PR as we discussed @maxscheurer . Since I'm not aware of another ptLR implementation, I validated the implementation by comparing to excitation energies from LR-PCM. For the Nitrobenzene Benchmark set I used to validate LR-PCM, the excitation energies agree up to 1e-3 Hartree. Additionally degenerate states also have degenerate corrections ;). So I would assume the implementation should be fine.

Concerning the tests... I'm not sure about the consistency test (compare to values calculated with the current versions of PSI4 and ADCc). Probably I should raise the tolerance, to avoid failures through some smaller, "irrelevant" changes in PSI4/PCMSolver. Also I'm not sure whether it is a good idea to add the generation of the Testdata to the generate.py script, since after running the script the consistency test will be passed 100%. Other than that I don't know at the moment what other tests to add, since PTE energies and reference values for corrections are not available.

[maxscheurer]

Can you do a rebase against the master branch? 😬

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 7a0e0e7f3722769255e26bcd4992d5f2cfed06b2 into fe3d42c7cfece3cb4d0e477c62166f8ff73cbc6c - view on LGTM.com

new alerts:

• 1 for Module imports itself

[maxscheurer]

BTW @jonasleitner feel free to add yourself here and here.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 697893cfe792bf36d7ec9d768654b8e66048d513 into fe3d42c7cfece3cb4d0e477c62166f8ff73cbc6c - view on LGTM.com

new alerts:

• 1 for Module imports itself

[maxscheurer]

The only things left to do are:

• Adding an example file for these features for each host programs for users to have a good starting point. Maybe create a folder named "environment" or so in examples and also move all the PE examples in there.

• Adding documentation for the new features. There is already something about PE in calculations.rst, so it would be easy to show one of the examples in there as well.

[mfherbst]

I'll try to do a review pass as well, might not get around to it before the weekend, though.

[jonasleitner]

Adding an example file for these features for each host programs for users to have a good starting point. Maybe create a folder named "environment" or so in examples and also move all the PE examples in there

You mean the two pna examples or is there anything else at another place?

[maxscheurer]

You mean the two pna examples or is there anything else at another place?

I think it's only the content of the pna folder...

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 0416b307088bb59c229a052b97d6cd563d5981c5 into fe3d42c7cfece3cb4d0e477c62166f8ff73cbc6c - view on LGTM.com

new alerts:

• 1 for Module imports itself

[jonasleitner]

Ok, beside the test calculations this should be pretty much it (and of course some typos probably :D).

[lgtm-com[bot]]

This pull request introduces 1 alert when merging de9897bc57c836674676c4b968d473c4d5ab4e4b into fe3d42c7cfece3cb4d0e477c62166f8ff73cbc6c - view on LGTM.com

new alerts:

• 1 for Module imports itself

[mfherbst]

I guess coverage goes down because the psi4 tests are not run, right?

[maxscheurer]

I guess coverage goes down because the psi4 tests are not run, right?

Yes, that's correct. The matrix setup etc. is however covered through pyscf tests.

[maxscheurer]

Once you've addressed Michael's comments, can you confirm that the Psi4 tests are working locally for you? Afterwards, I can merge the PR 👍

[jonasleitner]

Once you've addressed Michael's comments, can you confirm that the Psi4 tests are working locally for you?

Tests are running locally :+1:

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 689eaec7a36ba98936592aaf5624f2bf7c7c0fdc into fe3d42c7cfece3cb4d0e477c62166f8ff73cbc6c - view on LGTM.com

new alerts:

• 1 for Module imports itself

[maxscheurer]

Tests are running locally

Ok, so can I merge your first PR then? 😄

[jonasleitner]

Ok, so can I merge your first PR then?

Yes go on :+1: Everything should be fine :tada: