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

Make OEProp names flexible, to fix CC prop name bug. #2534

Merged
merged 3 commits into from
Apr 8, 2022

Conversation

JonathonMisiewicz
Copy link
Contributor

Description

This PR changes how OEProp saves variables and fixes a bug where "CC ROOT 0" variables were not being set.

Previously, OEProp overloaded title_ to refer to both the name used for the density matrix (for print purposes) and for the name used for properties (as a prefix for variable saving purposes). Only one such name can be used.
Now, OEProp uses the density matrix's name as the density matrix's name (for print purposes) and for the names used for properties (for variable saving purposes, and with the generality of format strings). Multiples names can be used.

With this, I can now save the CC dipoles as both "CC DIPOLE" and "CC2 DIPOLE", so "CC DIPOLE" can be found. The Psi code that tried to access this was never entered previously because it checked for a 'dipole' variable rather than a 'DIPOLE' variable.

Todos

  • More flexibility in OEProp names
  • Previously missing CC property variables are set

Checklist

  • Properties tests pass

Status

  • Ready for review
  • Ready for merge

@JonathonMisiewicz JonathonMisiewicz added this to the Psi4 1.6 milestone Apr 6, 2022
@JonathonMisiewicz JonathonMisiewicz added api Involves the Psi4 API. How do users access "stuff"? cc For all issues involving the CC module, ground-state energies to response properties. libmints For things that are wrong with libmints (but not wavefunction). labels Apr 6, 2022
tests/cc46/output.ref Outdated Show resolved Hide resolved
@JonathonMisiewicz
Copy link
Contributor Author

JonathonMisiewicz commented Apr 6, 2022

@loriab

Reverted the elimination of title_. Although I still don't believe it should exist, we need more density matrix standardization before I can safely eliminate it, and I can't standardize density matrices until after this PR is in.

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.

lgtm. thanks, and sorry it turned into a new level in the rabbit hole.

@JonathonMisiewicz
Copy link
Contributor Author

I'll give @jturney today to review this if he wants, in the name of libmints, but I'll merge this in tomorrow morning if I don't hear from him.

@JonathonMisiewicz JonathonMisiewicz merged commit 6447c91 into psi4:master Apr 8, 2022
@JonathonMisiewicz JonathonMisiewicz deleted the oeprop branch April 8, 2022 10:04
zachglick pushed a commit to zachglick/psi4 that referenced this pull request Apr 18, 2022
* Make OEProp names flexible, to fix CC prop name bug.

* Begrudgingly restore OEProp.title_

* Amend previous commit
zachglick pushed a commit to zachglick/psi4 that referenced this pull request Apr 18, 2022
* Make OEProp names flexible, to fix CC prop name bug.

* Begrudgingly restore OEProp.title_

* Amend previous commit
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. libmints For things that are wrong with libmints (but not wavefunction).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants