Skip to content

dfocc: df & cd remp & oremp methods #2670

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 22, 2022
Merged

dfocc: df & cd remp & oremp methods #2670

merged 1 commit into from
Sep 22, 2022

Conversation

loriab
Copy link
Member

@loriab loriab commented Aug 10, 2022 •
edited
Loading

Description

As it says on the tin.

ONLY REVIEW SECOND COMMIT Rebase req'd after PR No. 3 merged and before this merge.

This is PR No. 4 in the mega-dfocc-remp series.

Todos

  • REMP and OREMP added to dfocc along with 8 tests and stdsuite testing
  • general notations and eqn numbers from SB ported

Questions

  • @behnle the citation to which the eqn number comments refer you probably put in the bibliography, but it might be handy to have notated in the code

Checklist

Status

  • Ready for review
  • Ready for merge

@loriab loriab added this to the Psi4 1.7 milestone Aug 10, 2022
@loriab loriab added enhancement dfocc For issues in the DFOCC module. labels Aug 10, 2022
@loriab loriab mentioned this pull request Aug 10, 2022
Closed
8 tasks
@behnle
Copy link
Contributor

behnle commented Aug 10, 2022

@loriab The equation numbers refer to Phys. Chem. Chem. Phys., 2016, 18, 11362 (http://dx.doi.org/10.1039/C6CP00164E). This is all @bozkaya 's work, i just added them while trying to understand the code. These comments were originally not intended for the public 😁 .

@JonathonMisiewicz JonathonMisiewicz mentioned this pull request Aug 15, 2022
Merged
4 tasks
@loriab loriab force-pushed the df_cd_remp_oremp_2 branch from 265cc4a to 397a07c Compare August 19, 2022 16:04
@loriab
Copy link
Member Author

loriab commented Aug 19, 2022

Update: this is rebased and fit to review. @behnle, thanks for the comments on eqn numbers. I've added doi to the lines locally, but now they're all entangled with separate WIP.

@JonathonMisiewicz
Copy link
Contributor

Ping me for review after merge conflict resolve.

JonathonMisiewicz
JonathonMisiewicz reviewed Aug 30, 2022
View reviewed changes
Copy link
Contributor

@JonathonMisiewicz JonathonMisiewicz left a comment

Choose a reason for hiding this comment

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

I've given this a very rough first pass. There's so much technical debt in dfocc, I'm not quite sure how I want to review this one. Hopefully I'll figure it out on the second pass!

@loriab loriab force-pushed the df_cd_remp_oremp_2 branch from 4975dc4 to 465ffa3 Compare August 30, 2022 20:00
JonathonMisiewicz
JonathonMisiewicz reviewed Sep 6, 2022
View reviewed changes
Copy link
Contributor

@JonathonMisiewicz JonathonMisiewicz left a comment

Choose a reason for hiding this comment

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

A couple docs edits I'd request, but looks good enough to me otherwise. There needs to be some work in the module after the series concludes.

@loriab
Copy link
Member Author

loriab commented Sep 12, 2022

Eq numbers removed, rebased, and fully tested locally. Ready for review again.

JonathonMisiewicz
JonathonMisiewicz approved these changes Sep 12, 2022
View reviewed changes
Copy link
Contributor

@JonathonMisiewicz JonathonMisiewicz left a comment

Choose a reason for hiding this comment

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

Thanks much!

zachglick
zachglick approved these changes Sep 22, 2022
View reviewed changes
Copy link
Contributor

@zachglick zachglick 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 very familiar with REMP or dfocc but LGTM, aside from one small driver question.

psi4/src/psi4/dfocc/df_corr.cc
@@ -774,7 +774,7 @@ void DFOCC::b_so_non_zero() {
ndf_nz = 0;
#pragma omp parallel for
for (int Q = 0; Q < nQ; ++Q) {
for (int m = 0; m < nso_; ++m) {
for (int m = 0; m < nso_; ++m) { // for (int mn=0; mn < (nso_*nso_ - 1); ++mn){} ??
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like an equally fine way to do the double for loop.

@loriab loriab merged commit 765a1d3 into psi4:master Sep 22, 2022
@loriab loriab deleted the df_cd_remp_oremp_2 branch September 22, 2022 20:14
AlexHeide pushed a commit to AlexHeide/psi4 that referenced this pull request Sep 29, 2022
@loriab @behnle @AlexHeide
dfocc: df & cd remp & oremp (psi4#2670)
62be5f6 
Co-authored-by: stefan <stefan.behnle@uni-tuebingen.de>
@JonathonMisiewicz JonathonMisiewicz mentioned this pull request Feb 7, 2024
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zachglick zachglick zachglick approved these changes

@jturney jturney jturney approved these changes

@JonathonMisiewicz JonathonMisiewicz JonathonMisiewicz approved these changes

+1 more reviewer

@behnle behnle behnle left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
dfocc For issues in the DFOCC module. enhancement
Projects
None yet
Milestone
Psi4 1.7
Development

Successfully merging this pull request may close these issues.
5 participants
@loriab @behnle @JonathonMisiewicz @jturney @zachglick