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

Fix inconsistencies in formatted checkpoint files #1475

Merged
merged 11 commits into from Feb 4, 2019

Conversation

Projects
None yet
6 participants
@susilehtola
Copy link
Member

commented Jan 14, 2019

Description

The formatted checkpoint files currently produced by Psi4 misname the orbital coefficient and the density matrix fields. This PR (re)establishes the expected behavior, as well as fixes other bugs in the code.

Todos

Notable points (developer or user-interest) that this PR has or will accomplish.

  • Fix labels of MO energies and coefficients
  • Fix labels of density matrices
  • Fix printout of number of linearly independent functions
  • Don't print the MO coefficients into the log file
  • Only print out beta orbs and spin density if wave function is not spin-restricted.
  • Identify if density is correlated; if it is, print out both SCF and correlated density. Otherwise only print out SCF density.

Questions

  • Question1

Checklist

Status

  • Ready for review
  • Ready for merge

Susi Lehtola added some commits Jan 14, 2019

@JonathonMisiewicz

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

This is a partial fix to #1272.

Susi Lehtola added some commits Jan 14, 2019

@susilehtola

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

@JonathonMisiewicz thanks for pointing that out; I had a vague recollection of other problems existing in the code.

@JonathonMisiewicz

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

Are the densities supposed to always be SCF? The original reporter for #1272 was talking about densities for other methods, as well. Otherwise, I agree with your assessment that this will be a full fix to #1272. I'll check with the original reporter before closing.

@susilehtola

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

@JonathonMisiewicz AFAIK it's only printing out SCF densities. If there's a way to get unrelaxed MP2, CC, etc density matrices to print out in Psi4, that'd be great. In that case, one would just need to add in the correlated density matrices to print out on top of the SCF quantities.

@susilehtola

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

Nevermind:

molecule {
0 1
H          0.43774        1.07383       -0.88985
O         -0.04657       -0.75634        0.00000
H          0.86440       -1.05180        0.00000
}

set basis pcseg-1
set reference rhf
set s_tolerance 1e-5
set guess sad
scf_en, scf_wfn = energy('scf', return_wfn=True)
fchk(scf_wfn, "scf.fchk")
ccsd_en, ccsd_wfn = properties('ccsd', properties=['dipole'], return_wfn=True, ref_wfn=scf_wfn)
fchk(ccsd_wfn, "ccsd.fchk")

shows that the density matrix does change from SCF to CCSD.

Now, I would need a function in the Wavefunction class that would get me the SCF density matrix.

@susilehtola

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

... as well a way to check if the density matrix is correlated or not.

@susilehtola susilehtola changed the title Fix inconsistency in formatted checkpoint file Fix inconsistencies in formatted checkpoint files Jan 14, 2019

@loriab

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

@susilehtola there's https://github.com/psi4/psi4/blob/master/psi4/src/psi4/libmints/wavefunction.h#L352 but consider it API subject to change, as reference_wavefunction never really fulfilled its promise. On the other hand, if there's a test that uses that functionality, we can make sure it gets replaced with something that serves the same purpose.

Probably any wfn that has a ref_wfn is correlated.

@susilehtola

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2019

Well, the code is there, but it appears that reference_wavefunction() is returning me the correlated wave function. At least, the density matrix I get from refwfn coincides with the one from SCF, even though I know this should NOT be the case.

@JonathonMisiewicz

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2019

Apparently, the handling of correlated/non-correlated densities has been inconsistent for some time.

@dgasmith
Copy link
Member

left a comment

LGTM, thanks everyone!

@dgasmith
Copy link
Member

left a comment

Hmm, very odd that was on the wrong issue.

This has quite a ways to go.

@tovrstra

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2019

I just tested the code in this PR and works well for my purposes, i.e. loading the FCHK file into HORTON to perform a density partitioning. Thanks!

@susilehtola

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2019

I think this could be merged.

if (am == 4) {
for (int row = 0; row < 15; ++row)
for (int col = 0; col < 15; ++col) transmat->set(offset + row, offset + col, cartG[row][col]);
// Cartesians - S and P orbitals are fine, but higher terms need reordering

This comment has been minimized.

Copy link
@dgasmith

dgasmith Jan 31, 2019

Member

Why change this? It does not seem that you have altered the effect of the code but to use case which Psi4 stays away from.

This comment has been minimized.

Copy link
@susilehtola

susilehtola Jan 31, 2019

Author Member

The separate if clauses weren't aesthetic, so I wanted to use the syntax that exists in the language for this case.

Reverted to if else if.

int nbf = basis->nbf();
assert(Ca_ao->nrow() == nbf);

This comment has been minimized.

Copy link
@dgasmith

dgasmith Jan 31, 2019

Member

Any reason for the assert here, by definition within Psi4 this is true.

This comment has been minimized.

Copy link
@susilehtola

susilehtola Jan 31, 2019

Author Member

I guess I wanted to make sure in case Ca_ao was symmetry blocked. Removed

Susi Lehtola
@@ -25,6 +25,7 @@
*
* @END LICENSE
*/
#include <cassert>

This comment has been minimized.

Copy link
@dgasmith

dgasmith Feb 2, 2019

Member

Not needed I believe.

This comment has been minimized.

Copy link
@susilehtola

susilehtola Feb 2, 2019

Author Member

Removed.

// In theory, the correlated density should be printed out with a
// legend depending on the method (MP2, MP3, MP4, CI, CC) but this
// is probably the most common anyhow
if (pHF) write_sym_matrix("Total CC Density", reorderedDPHFt);

This comment has been minimized.

Copy link
@dgasmith

dgasmith Feb 2, 2019

Member

This isn't just the CC density? Could be many theories.

This comment has been minimized.

Copy link
@loriab

loriab Feb 2, 2019

Member

This looks like a description of the fchk format that might have been in an earlier version of the Gaussian manual. I can't find similar in current docs.

The headers for Total <mtd> Density are so general (SCF, MP2, CI, CC) that CC may be reasonable. Or, since this is called from py, can pull name off wfn, regex it into SCF/MP2/CI/CC and pass that label into the fchk writer. In any case, the 2nd line of the fchk, Type, Method, Basis, provide a place to put the proper, e.g., CISD method.

Other than tidying up the name, lgtm! oh, and should probably check that gdma is content with the changes. Thanks for fixing this up.

This comment has been minimized.

Copy link
@susilehtola

susilehtola Feb 2, 2019

Author Member

As discussed elsewhere, this is just an aesthetic issue. Since the checkpoint never includes other than the SCF and one unrelaxed post-HF density, it doesn't really matter how the post-HF density is named - as long as the name doesn't violate the standard. Since >99% of use cases are likely CC, I don't see why this would need to be changed here.

Susi Lehtola and others added some commits Feb 2, 2019

Susi Lehtola
@loriab

loriab approved these changes Feb 2, 2019

Copy link
Member

left a comment

I added a commit with some notes. Very glad to have this patch up. Thanks!

@loriab loriab removed the backport label Feb 2, 2019

@loriab loriab modified the milestones: Psi4 1.4, Psi4 1.3 Feb 2, 2019

@dgasmith
Copy link
Member

left a comment

LGTM

@susilehtola

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2019

@jturney

jturney approved these changes Feb 4, 2019

@jturney jturney merged commit 5c0e495 into psi4:master Feb 4, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
psi4.psi4 #20190202.9 succeeded
Details

@susilehtola susilehtola deleted the susilehtola:fchk_fixes branch Feb 4, 2019

@loriab loriab referenced this pull request Feb 5, 2019

Closed

gdma and fchk #1523

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.