Skip to content

Refactor JK_deriv2 #2994

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

Merged
merged 1 commit into from
Jun 28, 2023

Conversation

JonathonMisiewicz
Copy link
Contributor

@JonathonMisiewicz JonathonMisiewicz commented Jun 27, 2023

Description

The JK_deriv2 function exists to compute the overlap-derivative-times-TEI part of the right side of the CPHF equation. It's a J-term and a K-like term. with overlap derivative integrals instead of a density.[1] The previous algorithm to do this first computed the alpha spin part in one function call, and then the beta spin part in another function call. This approach was redundant. The first function call had all the intermediates necessary to compute the beta part, but didn't use them. As a result, the function re-computed JK.

This PR refactors JK_deriv2 so it computes both spin cases in a single function call.

[1] = There's also a Vx term, but its implementation was both unused and buggy. Now it's just unused. Trying to use it in the old way would be even more redundant.

User API & Changelog headlines

  • The UHF hessian algorithm has been slightly adjusted, which should lead to slightly faster computations. Please report any errors.

Dev notes & details

  • Refactored JK_deriv2 to compute both spin cases in a single function call.
  • Makes UKS LDA hessians much less ugly.
  • Makes JK_deriv2 comply with compute_Vx's expected function signature

Checklist

  • ctest -R scf-hess passes

Status

  • Ready for review
  • Ready for merge

@JonathonMisiewicz JonathonMisiewicz added this to the Psi4 1.9 milestone Jun 27, 2023
Copy link
Member

@jturney jturney left a comment

Choose a reason for hiding this comment

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

Yeah looks cleaner

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.

Looks reasonable (and a lot more readable) to me. I'm curious if there was an intended generalization by the c1/c2 setup.

// temporary intermediates in a loop. In the (likely) event the allocation time cost is
// negligible in comparison to the J/K build, simplify away.
C_DGEMM('N','N',nso,naocc,nso,1.0,J[2*a]->pointer()[0],nso,Caop[0],naocc,0.0,Tap[0],naocc);
C_DGEMM('T','N',nmo,naocc,nso,1.0,Cap[0],nmo,Tap[0],naocc,0.0,Uap[0],naocc);
Copy link
Member

Choose a reason for hiding this comment

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

Change of constants from -1,1 to 1,0 deliberate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

In the old code, we backtransform four times and then add the pieces together. In the new code, we add the four pieces together and backtransform once. The job that those constants used to do is now fulfilled by J[2*a]->add(J[2*a+1]); J[2*a]->scale(-1); a few lines above.

@JonathonMisiewicz
Copy link
Contributor Author

Looks reasonable (and a lot more readable) to me. I'm curious if there was an intended generalization by the c1/c2 setup.

Yes, and getting rid of that intended generalization is precisely the point of this PR.

The intended generalization of the c1/c2 setup was that you could pass in alpha and then beta to compute the alpha block, or you could pass in beta and then alpha to compute the beta block. We're now computing both at once, so we don't need that. Having c1/c2 now is just confusing. Instead, we make explicit that alpha comes before beta.

@JonathonMisiewicz JonathonMisiewicz added this pull request to the merge queue Jun 28, 2023
Merged via the queue into psi4:master with commit 6c9daf4 Jun 28, 2023
@JonathonMisiewicz JonathonMisiewicz mentioned this pull request Jun 29, 2023
6 tasks
@JonathonMisiewicz JonathonMisiewicz deleted the refactor_jkderiv2 branch June 29, 2023 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants