Add adcc harness

[maxscheurer]

Description

This PR adds a harness for adcc, a Python-driven toolkit for excited states within the algebraic-diagrammatic construction scheme for the polarization propagator.

adcc requires an SCF driver to run first (e.g. Psi4, pyscf) to provide the SCF reference state, integrals, etc. throughout the rest of the computation.

CC @mfherbst

ToDo

• [x] robust SCF wrapper
• [x] proper input keyword handling (including defaults etc.)
• [x] error handling
• [x] add tests

Changelog description

Add a harness for adcc

Status

• [x] Code base linted
• [x] Ready to go

[lgtm-com[bot]]

This pull request introduces 12 alerts when merging 736b99a91c6d25a0f4be9429216d5927196a6bf1 into a206fb9f1a5d214b67e2a7124771a688a4418501 - view on LGTM.com

new alerts:

• 9 for Unused import
• 2 for Unused local variable
• 1 for Module is imported more than once

[lgtm-com[bot]]

This pull request introduces 12 alerts when merging b8ff9f92e8fbd9bf8dffd5eebc76c82755ccef33 into a206fb9f1a5d214b67e2a7124771a688a4418501 - view on LGTM.com

new alerts:

• 9 for Unused import
• 2 for Unused local variable
• 1 for Module is imported more than once

[lgtm-com[bot]]

This pull request introduces 10 alerts when merging 42d25c3392852286b5241ff17b8b2b31565de47a into a206fb9f1a5d214b67e2a7124771a688a4418501 - view on LGTM.com

new alerts:

• 9 for Unused import
• 1 for Unused local variable

[lgtm-com[bot]]

This pull request introduces 10 alerts when merging 47906258e0e0e1f5000d5985b5ffb82c895a50c9 into a206fb9f1a5d214b67e2a7124771a688a4418501 - view on LGTM.com

new alerts:

• 9 for Unused import
• 1 for Unused local variable

[maxscheurer]

I think the next step would be to discuss how/which input options are transported to the run_adc function, and what the preferred naming convention in input_model.keywords, @loriab.

Our documentation of run_adc can be found here.

[maxscheurer]

@mfherbst, I gave you write access to my fork, so you can also make changes 😉

[lgtm-com[bot]]

This pull request introduces 11 alerts when merging 7ffae0dcf9dee699c066fcb055f4a228a24c113d into a206fb9f1a5d214b67e2a7124771a688a4418501 - view on LGTM.com

new alerts:

• 10 for Unused import
• 1 for Unused local variable

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 8ca1a865217ebe6b4655a1bc9d052cddf4085a62 into a206fb9f1a5d214b67e2a7124771a688a4418501 - view on LGTM.com

new alerts:

• 1 for Unused import

[maxscheurer]

Any hints on how to fix the CI, @loriab?

[loriab]

Any hints on how to fix the CI, @loriab?

I think try working from psi-nightly.yaml rather than psi.yaml. The latter is also serving as a "minimum working versions" CI and so has some = instead of >=. I think that's what's making it select psi4.dev3xx rather than psi4.dev10xx.

[codecov[bot]]

Codecov Report

Merging #277 (6d08ab6) into master (3cbe78b) will increase coverage by 0.50%. The diff coverage is 90.90%.

[mfherbst]

Hooray! CI passes. I guess what is left in terms of functionality is to extend the data / property extraction and get that into adcc? Then of course proper tests to wrap it up?

[maxscheurer]

What other properties do you have in mind? I think what is exported right now is equivalent to the psi4 vars.

Regarding tests, I would suggest small functionality tests like in Psi4, what do you think?

[loriab]

if these are the kinds of molecules you'd be using for testing anyway, I suggest the following mol/ref/basis combinations: https://github.com/MolSSI/QCEngine/blob/master/qcengine/programs/tests/standard_suite_ref.py#L9-L77 . If they don't serve your purposes, disregard.

[maxscheurer]

Thanks, @loriab! I was thinking we could also use the molecules/etc that we already have reference data for, but I have no strong opinion on this. If it's better to use the standard_suite_ref molecules, I can also generate data for those.

[mfherbst]

Since we are not really testing the ADC implementation, I think there is no real reason to reuse the molecules we have data for (which where chosen to provide test cases for specific aspects of our implementation). So I don't really have a strong opinion either. Perhaps we should just use whatever is easiest, i.e. the molecules we have data for? Or is it because of maintainability beneficial to stick to the standard_suite_ref molecules @loriab? In that case I would say we do the latter.

[mfherbst]

Sorry about the long delay in continuing this PR. We had a few issues with our deployment moving from Travis to GHA. It turned out that the easiest solution was to work around the remaining proprietary bits of adcc and make the code completely open-source, so now we can hopefully finish this up and get it in.

[mfherbst]

@maxscheurer I'm on it.

[mfherbst]

@maxscheurer Seems like we are not exporting any of the computed properties. Any idea why?

[maxscheurer]

I think the computed data should be in ret["extras"]["qcvars"], no?

[mfherbst]

Hmm ok, but I'm not sure that is all that reasonable, don't you think?

[maxscheurer]

I think since these are "custom" variables (i.e. not following QCSchema), this is the intended spot for the results to live right now. What do you think, @loriab?

[mfherbst]

Cool, thanks @maxscheurer !

[loriab]

add yourselves to codemeta, too, if you both like. here's a start:

        {
            "@type": "Person",
            "givenName": "Maximilian",
            "familyName": "Scheurer",
            "affiliation": "Interdisciplinary Center for Scientific Computing, Heidelberg University",
            "@id": "https://orcid.org/0000-0003-0592-3464",
            "identifier": "https://github.com/maxscheurer"
        },
        {
            "@type": "Person",
            "givenName": "Michael",
            "additionalName": "F.",
            "familyName": "Herbst",
            "affiliation": "Interdisciplinary Center for Scientific Computing, Heidelberg University",
            "@id": "https://orcid.org/0000-0003-0378-7921",
            "identifier": "https://github.com/mfherbst"
        },

[maxscheurer]

I've addressed the comments and added myself to codemeta. @mfherbst please add yourself because I don't know your affiliation 😄

[mfherbst]

I don't know your affiliation

For the next 2 weeks still France, then Aachen ;). I'll add what it is now and update later.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 75257ce46eac4448de35088cd7158c0c1cf7ca41 into 6adbccc2a3d982c667ae313c60d53e25ca53e34e - view on LGTM.com

new alerts:

• 1 for Unused import

[maxscheurer]

I'm happy with the harness now 😄 You can squash-merge this, if you want, @loriab.