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

Renaming of excited states dipoles #2541

Merged
merged 5 commits into from Apr 18, 2022

Conversation

JonathonMisiewicz
Copy link
Contributor

Description

More cc tests ported over. There's one autotest test left.

Todos

  • Documented several CC psivars
  • Updated dipole size managing functions to work with new "DIPOLE -" syntax
  • cc density variables renamed to new syntax
  • cc dipole/quadrupole variables renamed to new syntax

Checklist

  • ctests pass

Status

  • Ready for review
  • Ready for merge

@JonathonMisiewicz JonathonMisiewicz added testing api Involves the Psi4 API. How do users access "stuff"? cc For all issues involving the CC module, ground-state energies to response properties. labels Apr 9, 2022
@JonathonMisiewicz JonathonMisiewicz added this to the Psi4 1.6 milestone Apr 9, 2022
@JonathonMisiewicz
Copy link
Contributor Author

JonathonMisiewicz commented Apr 11, 2022

I'm all for keeping them [excited-state densities] around, just not in qcvars, since those get exported and printed thoughtlessly. I'd favor a different member data on Wfn. Others thoughts?

I hope you know what you're asking for. Storage of ground-state densities is lawless, let alone excited-state densities.

For excited-state densities...

  • ADC won't export densities of any sort.
  • TD-DFT will save left and right eigenvectors. From my (very shaky) knowledge of TD-DFT theory, those are left and right transition densities.
  • EOM-CC creates but will not save its left and right transition densities. Before this PR, it would write excited state densities to the wavefunction. (In the current version of the PR, it still does)

For ground-state densities...

  • For fully variational methods (including in orbitals), the density of the correlated method is unambiguous and saved to the wavefunction.
  • For methods that are not fully variational, you have orbital-relaxed and orbital-unrelaxed densities. If orbital relaxation is not needed, this density may or may not get saved to the wavefunction. If it is not saved, the density is probably still the SCF density.
  • Some methods have it even worse. For example, ODC-12 with perturbative lambda has three conceivable densities. ODC-12 density, ODC-12 density + perturbative lambda, and ODC-12 + perturbative lambda + orbital relaxtion.
  • CAS-DMRG-PT2 through the dmrg module saves the DMRG density to the wavefunction, which is not the DMRGPT2 density.
  • If the user requests a spin-scaled density, the standard densities need to be further monkeyed with to be correct in gradients. I don't know if this is actually done. I suspect it is not.

And to represent all that, the current tools we have are Da, Db, and the pool of psivars.

I'm in favor of deprecating Da and Db for anything other than internal SCF use and creating a new wavefunction member to store all the densities, with proper labeling. This will of course be a lot of code refactoring, but that is quite literally the entire point of me doing all this cc PRs. And of course, somebody will need to tell dfocc about this API change. 🙂

@loriab
Copy link
Member

loriab commented Apr 11, 2022

I'm in favor of deprecating Da and Db for anything other than internal SCF use and creating a new wavefunction member to store all the densities, with proper labeling. This will of course be a lot of code refactoring, but that is quite literally the entire point of me doing all this cc PRs. And of course, somebody will need to tell dfocc about this API change. 🙂

Thanks for the enumeration of the organizational state of all the densities. The new Wfn member for finalized, labeled densities sounds good to me. Are the ES CC densities ready for that state yet, or are they still in preparation? For any that are still in preparation but still in need of saving, I guess I propose a temporary member of Wfn that is marked as having a finite lifetime. Same as Wfn.arrays, just not Wfn.arrays :-) Ben's already nervous about storing the per-atom charges that end up in qcvars, so I'm just not in favor of densities there.

@JonathonMisiewicz
Copy link
Contributor Author

The CC excited state densities are most definitely correct. If they weren't, EOM analytic gradients would be wrong, and cc23 would fail. I would expect that correctness is enough to finalize these, but I defer to our resident Crawdad on this.

I don't know whether the transition densities pass some kind of finite difference test, but I am prepared to save that question for another day.

@lothian
Copy link
Contributor

lothian commented Apr 11, 2022

Can you clarify what you mean about a "finite difference test" for the transition densities?

@JonathonMisiewicz
Copy link
Contributor Author

JonathonMisiewicz commented Apr 11, 2022

By finite-difference test, I mean "there is some property that we can compute either by finite difference of energies or by contracting appropriately defined densities against derivative integrals," e.g., geometry gradients and dipoles. By checking that both routes predict the same result, we can be much more confident that the densities are correctly implemented. (I recall a sign error in the CASPT2 gradients of another package. This error went uncaught for decades because the impact on calculations was relatively small.)

I don't know if this is an option for transition densities. I'm not sure if EOM-CC transition properties are defined by some variational criteria, some variational criteria but neglecting orbital relaxation, or something else altogether.

EDIT: The '93 Stanton and Bartlett paper explicitly says orbital relaxation is neglected, so I imagine the finite difference test is not an option. I'm not sure if there's some other technique to validate the correctness of the transition densities, other than matching other code. I know "matching other code" is done in the test suite.

@lothian
Copy link
Contributor

lothian commented Apr 11, 2022

OK, then we're on the same page. Transition properties aren't clearly defined as derivatives, so I don't see how a finite-difference approach can help in this case, but we might be able to identify some limiting cases. But I agree it can/should wait for another day.

Copy link
Member

@loriab loriab left a comment

Choose a reason for hiding this comment

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

apart from lodging the root densities outside Wfn.arrays, lgtm!

psi4/driver/procrouting/proc.py Outdated Show resolved Hide resolved
tests/cc46/input.dat Show resolved Hide resolved
tests/cc48/input.dat Show resolved Hide resolved
@JonathonMisiewicz
Copy link
Contributor Author

Okay, latest version of the code moves the excited state densities to a new field. Unless anybody has grievances, we'll start trying this out.

/// Vector of alpha density matrices
std::map<std::string, SharedMatrix> Da_map_;
/// Vector of beta density matrices
std::map<std::string, SharedMatrix> Db_map_;
Copy link
Member

Choose a reason for hiding this comment

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

if Da_map_ and Db_map_ become the long-term holders, should probably add some of the key.upper() features of variables and arrays, but that's for the beyond experimental stage.

Copy link
Member

@andysim andysim left a comment

Choose a reason for hiding this comment

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

I'm not really qualified to appraise this, but I don't see any obvious errors.

@JonathonMisiewicz
Copy link
Contributor Author

I'm not really qualified to appraise this, but I don't see any obvious errors.

Alas, nobody is fully qualified to appraise this. TDC knows the cc code and Lori knows psivars, but densities touch so many parts of the code that we'd need expertise in every single module, which we simply don't have.

I won't be free to work on the next PR for a while, so we can leave this open to collect more comments.

@JonathonMisiewicz JonathonMisiewicz merged commit 42f7d39 into psi4:master Apr 18, 2022
@JonathonMisiewicz JonathonMisiewicz deleted the cctest branch April 18, 2022 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Involves the Psi4 API. How do users access "stuff"? cc For all issues involving the CC module, ground-state energies to response properties. testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants