DFJK Expert I/O Control Bonus: PKJK #2926

davpoolechem · 2023-04-21T17:01:22Z

Description

This PR is a companion PR to #2848 and #2924. Those two PRs introduced the SCF_SUBTYPE keyword to Psi4, allowing for specifications of subalgorithms in SCF methods where appropriate, and applied the SCF_SUBTYPE keyword to MemDFJK and DiskDFJK, respectively.

As it turns out, the DFJK algorithms aren't the only JK algorithms where this new keyword may be useful.

The PKJK algorithm contains a number of subalgorithms, as well - an in-core algorithm (labeled InCore in the code) and two out-of-core algorithms (labeled Reorder and Yoshimine in the code). PKJK has some amount of control over which subalgorithm is utilized already - the PK_ALGO keyword specifies whether the Reorder or Yoshimine out-of-core algorithm is to be used, if an out-of-core algorithm is used; while the PK_NO_INCORE keyword disables the InCore PK algorithm and forces one of the out-of-core algorithms to be used. If the functionality of PK_NO_INCORE seems familiar, that's because it is - it is effectively the exact same thing as SCF_SUBTYPE=OUT_OF_CORE for MemDFJK (in #2848) and DiskDFJK (in #2924).

The goal of this PR is to clean that up; PK_NO_INCORE is replaced by SCF_SUBTYPE for PKJK. SCF_SUBTYPE=OUT_OF_CORE has the exact same effect that PK_NO_INCORE=TRUE had previously. SCF_SUBTYPE=AUTO has PKJK select in-core or out-of-core by default, as per usual. Finally, SCF_SUBTYPE=INCORE allows PKJK to force-execute its in-core algorithm, a new functionality for PKJK as far as I am aware. As usual with SCF_SUBTYPE, setting it to INCORE without allocating sufficient memory to Psi4 will throw an exception.

User API & Changelog headlines

The PK_NO_INCORE keyword has been removed.
The SCF_SUBTYPE keyword has been added for PKJK. SCF_SUBTYPE=AUTO has PKJK select the subalgorithm by default. SCF_SUBTYPE=OUT_OF_CORE forces PKJK to use one of the two out-of-core algorithms (equivalent to PK_NO_INCORE=TRUE previously). SCF_SUBTYPE=INCORE forces PKJK to use its in-core algorithm, and throws an exception if insufficient memory is allocated.

Dev notes & details

The PK_NO_INCORE keyword for PKJK has been replaced with the new SCF_SUBTYPE keyword.

Questions

-N/A

Checklist

Tests added for any

new features

All or relevant fraction of full tests run

Status

Ready for review
Ready for merge

loriab · 2023-04-24T07:04:30Z

Nice idea to apply SCF_SUBTYPE to PK, too. Only thing that concerns me is that now selecting the algorithm (which most ppl won't) is a three-keyword affair: scf_type=pk, scf_subtype=out_of_core, pk_algo=yoshimine. What do you/others think of dropping pk_algo and having several scf_subtype options applicable to PK: AUTO (of course), INCORE, YOSHIMINE_OUT_OF_CORE, REORDER_OUT_OF_CORE, OUT_OF_CORE?

davpoolechem · 2023-04-24T12:06:53Z

Nice idea to apply SCF_SUBTYPE to PK, too. Only thing that concerns me is that now selecting the algorithm (which most ppl won't) is a three-keyword affair: scf_type=pk, scf_subtype=out_of_core, pk_algo=yoshimine. What do you/others think of dropping pk_algo and having several scf_subtype options applicable to PK: AUTO (of course), INCORE, YOSHIMINE_OUT_OF_CORE, REORDER_OUT_OF_CORE, OUT_OF_CORE?

Thank you! I had thought about doing this very thing, and I am fine with it if everyone else is. My initial hesitation in doing so, is that YOSHIMINE_OUT_OF_CORE and REORDER_OUT_OF_CORE are completely inapplicable to the DF algorithms; whereas with the current arrangement, all of the options in PK_ALGO and SCF_SUBTYPE can be used with their respective applicable algorithms.

Practically speaking, it's not a huge problem, since MemDFJK and DiskDFJK already throw exceptions for incompatible SCF_SUBTYPE options (as YOSHIMINE_OUT_OF_CORE and REORDER_OUT_OF_CORE would be). It just feels a bit... not ideal to me, I guess? If this option is considered better than the three-keyword approach currently used, I'll be happy to implement it, however!

loriab · 2023-04-24T17:54:19Z

The some-subtype-values-not-applicable-to-some-scftype-values doesn't bother me too much -- after all, direct, cd, etc. can't take any subtype value besides auto. So long as the read_options docstring is clear and the code rejects non-meaningful subtype choices, I don't think users are led astray.

All the same, I'm not hearty on my suggestion because it seems a little indistinct between algorithm choice (usually scftype) and memory transition choice (usually subtype). Maybe, if designed from scratch, there'd be YOSHIMINE_PK, REORDERED_PK, and PK as scftypes and then PK subtypes OUT_OF_CORE and INCORE would fit the DF pattern nicely. That might well be considered too much reengineering at this point in the release cycle.

So minimum change, unless others chime in, is leave as-is and add the PK_NO_INCORE to the deprecation messages here https://github.com/psi4/psi4/blob/master/psi4/src/core.cc#L553

davpoolechem · 2023-04-24T18:22:59Z

The some-subtype-values-not-applicable-to-some-scftype-values doesn't bother me too much -- after all, direct, cd, etc. can't take any subtype value besides auto. So long as the read_options docstring is clear and the code rejects non-meaningful subtype choices, I don't think users are led astray.

That's all fair, to be honest, although I agree moreso with your second paragraph below, and what you said about indistinctness between algorithm choice and memory transition choice is applicable for sure.

All the same, I'm not hearty on my suggestion because it seems a little indistinct between algorithm choice (usually scftype) and memory transition choice (usually subtype). Maybe, if designed from scratch, there'd be YOSHIMINE_PK, REORDERED_PK, and PK as scftypes and then PK subtypes OUT_OF_CORE and INCORE would fit the DF pattern nicely. That might well be considered too much reengineering at this point in the release cycle.

This is true, and a point I hadn't considered, either. I wouldn't mind taking such a project on, but I would probably save it for after v1.8.

So minimum change, unless others chime in, is leave as-is and add the PK_NO_INCORE to the deprecation messages here https://github.com/psi4/psi4/blob/master/psi4/src/core.cc#L553

Will do!

davpoolechem · 2023-04-25T13:07:20Z

Deprecation message added! I'm less familiar with deprecating things in Psi4, so let me know if anything else should be done on that front.

JonathonMisiewicz · 2023-04-25T18:38:17Z

psi4/src/read_options.cc

        /*- Maximum numbers of batches to read PK supermatrix. !expert -*/
        options.add_int("PK_MAX_BUCKETS", 500);
        /*- Select the PK algorithm to use. For debug purposes, selection will be automated later. !expert -*/
        options.add_str("PK_ALGO", "REORDER", "REORDER YOSHIMINE");


After this PR, does this keyword name make sense?

I think so -- it's still the only way to choose btwn reorder and yoshimine when scf_type=PK. Am I missing your point?

EDIT: docstring might be edited to mention if you want PK+INCORE, use subtype.

This is pretty similar in topic to what @loriab had asked in her review.

It doesn't make any less sense than before, since PK_ALGO does exactly what it did previously - it controls what out-of-core algorithm is selected by PKJK, if an out-of-core algorithm is to be used. Previously, the way one would force an out-of-core algorithm to be used would be to set PK_NO_INCORE = TRUE. Now, out-of-core can be forced by SCF_SUBTYPE = OUT_OF_CORE.

If PK is the algorithm and the subalgorithm is in core or out-of-core, is this a sub-sub-algorithm?

EDIT: docstring might be edited to mention if you want PK+INCORE, use subtype.

This is a good idea! I'd be happy to put that in.

is this a sub-sub-algorithm?

Yeah, that's what I didn't like about it. But the most logical fix I could devise (see above) is expanding scf_type, so this is the least intrusive for now.

What about collapsing PK_ALGO into SCF_SUBTYPE? So for PK, allowed options are INCORE, OOC_REORDER, and OOC_YOSHIMINE?

What about collapsing PK_ALGO into SCF_SUBTYPE? So for PK, allowed options are INCORE, OOC_REORDER, and OOC_YOSHIMINE?

This is a possibility I had thought of. I was originally concerned about it because keywords like OOC_REORDER and OOC_YOSHIMINE wouldn't apply to MemDFJK and DiskDFJK, which SCF_SUBTYPE also controls (although I've been convinced otherwise about this being a significant issue). There is also the issue that SCF_SUBTYPE has a specific intent currently of controlling in-core vs. out-of-core; and adding Yoshimine and Reorder keywords to SCF_SUBTYPE muddies the waters in that regard, since it would become unclear whether SCF_SUBTYPE deals with more specific sub-algorithm choice, or simply choice of in-core vs. out-of-core.

If there's enough support for folding PK_ALGO into SCF_SUBTYPE, however, I wouldn't mind doing that for this PR. I would also be happy to tackle Lori's proposal, but I'd rather that be for another PR.

I'm still for folding PK_ALGO into SCF_SUBTYPE, thanks for considering.

Agreed that Lori's proposal is beyond-scope.

davpoolechem · 2023-04-26T15:22:16Z

Hi all! I was working on folding the PK_ALGO keywords into SCF_SUBTYPE, and I ran into an issue that may be worth discussing.

By default, using OOC_YOSHIMINE and OOC_REORDER as SCF_SUBTYPEs the way we were discussing causes the calculation to throw an exception. The issue is because, in the SAD guess, DFJK is used by default; and SCF_SUBTYPE applies to the SAD guess DFJK (at least MemDFJK currently, and soon DiskDFJK) as well. Since OOC_YOSHIMINE and OOC_REORDER would be used as the keyword for the SAD DFJK, and since DFJK doesn't support those keywords explicitly for SCF_SUBTYPE, the calculation throws an exception. Note that setting SAD_SCF_TYPE=DIRECT eliminates this issue entirely.

There are probably a few ways to resolve this, if we choose to explicitly continue on this path:

As mentioned above, simply force SAD_SCF_TYPE to another option (like DIRECT) for PK calculations. I'll admit I'm not a big fan of this approach since DF is more performant, although it may not matter in the grand scheme of things since it's just the SAD guess.
Force OOC_YOSHIMINE and OOC_REORDER to work with the DFJK algorithms. The logical way to do this would be to have those keywords act the exact same as SCF_SUBTYPE=OUT_OF_CORE for DFJK. I'm a bit iffy on this one, as well, since it may imply that the three out-of-core keywords have different impacts on DFJK when they really don't. Documentation can clear that up, but still.

What does everyone think?

loriab · 2023-04-26T15:30:42Z

If it's not awful to implement, I'd make the SAD guess impervious to SCF_SUBTYPE. The user setting SCF_SUBTYPE is thinking about the main iterations, not the guess, and if this is some highly technical timing, the guess type might be reset anyways.

Also, I think it should be REORDER_OUT_OF_CORE not REORDER_OOC for consistency with the other values.

davpoolechem · 2023-04-26T15:34:07Z

If it's not awful to implement, I'd make the SAD guess impervious to SCF_SUBTYPE. The user setting SCF_SUBTYPE is thinking about the main iterations, not the guess, and if this is some highly technical timing, the guess type might be reset anyways.

Yeah, I'd agree with this. I can look into it and see if it's a quick fix or not.

Also, I think it should be REORDER_OUT_OF_CORE not REORDER_OOC for consistency with the other values.

Yeah, I'll do that, as well!

davpoolechem · 2023-04-26T20:21:52Z

All right, I disabled SCF_SUBTYPE for SAD guess runs! I went with a pretty basic solution - just adjust the Python driver and force-set SCF_SUBTYPE=AUTO for the guess part of the run. Let me know if this is an acceptable solution.

loriab · 2023-04-26T20:33:02Z

All right, I disabled SCF_SUBTYPE for SAD guess runs! I went with a pretty basic solution - just adjust the Python driver and force-set SCF_SUBTYPE=AUTO for the guess part of the run. Let me know if this is an acceptable solution.

Ah, turning it off py-side is simple and great, but there's a better way. The minor trouble with your approach is that it's restoring the value but not the status of whether the user has set it. That latter piece of info is often used to decide whether an option is default or not or whether, once control has returned to the user, to write the option explicitly in subsequent calcs. The usual remedy that you'll see scattered in proc.py are optstash=OptionsState([list o options]) ... do work ... optstash.restore(). So I'd suggest using that or the content manager like https://github.com/psi4/psi4/pull/2926/files#diff-2aec42d854b50db94cde6a3f9d3c665eed11e8f1f083040807d9e4dd4696895aL67-L68 ["SCF", "SCF_SUBTYPE"] for "local" setting.

davpoolechem · 2023-04-26T21:34:06Z

All right, I disabled SCF_SUBTYPE for SAD guess runs! I went with a pretty basic solution - just adjust the Python driver and force-set SCF_SUBTYPE=AUTO for the guess part of the run. Let me know if this is an acceptable solution.

Ah, turning it off py-side is simple and great, but there's a better way. The minor trouble with your approach is that it's restoring the value but not the status of whether the user has set it. That latter piece of info is often used to decide whether an option is default or not or whether, once control has returned to the user, to write the option explicitly in subsequent calcs. The usual remedy that you'll see scattered in proc.py are optstash=OptionsState([list o options]) ... do work ... optstash.restore(). So I'd suggest using that or the content manager like https://github.com/psi4/psi4/pull/2926/files#diff-2aec42d854b50db94cde6a3f9d3c665eed11e8f1f083040807d9e4dd4696895aL67-L68 ["SCF", "SCF_SUBTYPE"] for "local" setting.

Done and done! This is all great knowledge; thank you!

psi4/driver/procrouting/scf_proc/scf_iterator.py

psi4/src/read_options.cc

psi4/src/core.cc

psi4/src/psi4/libfock/PKmanagers.cc

loriab

thanks! much easier to read.

davpoolechem · 2023-04-28T12:06:48Z

So now that this PR and #2924 have both been approved for adding to the merge queue, my guess is that there will be a merge conflict between the two, at the SCF_SUBTYPE docstring in scf_subtype.cc.

loriab · 2023-04-28T14:59:29Z

So now that this PR and #2924 have both been approved for adding to the merge queue, my guess is that there will be a merge conflict between the two, at the SCF_SUBTYPE docstring in scf_subtype.cc.

Sounds worth adding these two to the queue and seeing how it handles the second. Agree that probably you'll have to rebase or reconcile somehow.

davpoolechem · 2023-04-28T15:07:50Z

So now that this PR and #2924 have both been approved for adding to the merge queue, my guess is that there will be a merge conflict between the two, at the SCF_SUBTYPE docstring in scf_subtype.cc.

Sounds worth adding these two to the queue and seeing how it handles the second. Agree that probably you'll have to rebase or reconcile somehow.

Yeah, I don't think we've had an actual merge conflict from the merge queue yet (at least not that I have seen), so it can be an experiment to see what happens in that case.

Co-authored-by: Lori A. Burns <[email protected]>

davpoolechem · 2023-04-28T18:55:15Z

I suppose if anyone wants to check the merged SCF_SUBTYPE docstring, they can; else, I'll add this to the merge queue by the time I finish up today.

loriab added this to the Psi4 1.8 milestone Apr 24, 2023

loriab added the scf Involves general SCF: convergence algorithms, RHF/UHF/ROHF/CUHF... label Apr 24, 2023

loriab approved these changes Apr 25, 2023

View reviewed changes

JonathonMisiewicz reviewed Apr 25, 2023

View reviewed changes

loriab reviewed Apr 26, 2023

View reviewed changes

psi4/driver/procrouting/scf_proc/scf_iterator.py Show resolved Hide resolved

loriab requested changes Apr 26, 2023

View reviewed changes

psi4/src/read_options.cc Outdated Show resolved Hide resolved

psi4/src/core.cc Show resolved Hide resolved

psi4/src/psi4/libfock/PKmanagers.cc Show resolved Hide resolved

psi4/src/psi4/libfock/PKmanagers.cc Outdated Show resolved Hide resolved

loriab approved these changes Apr 27, 2023

View reviewed changes

jturney approved these changes Apr 28, 2023

View reviewed changes

davpoolechem added this pull request to the merge queue Apr 28, 2023

github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Apr 28, 2023

David Poole added 6 commits April 28, 2023 14:30

Initial replacement of PK_NO_INCORE with SCF_SUBTYPE

117f6f3

Extend SCF_SUBTYPE capabilities in PK

3942b58

Small code refactor

ee7790e

Whitespace cleanup

0b85b83

Final whitespace cleanup

4e96880

Update SCF_SUBTYPE comment to include PK

fc5432c

David Poole and others added 11 commits April 28, 2023 14:31

Add deprecation message for PK_NO_INCORE

cdc291f

Update PK_ALGO docstring in read_options

082d805

Fold PK_ALGO keywords into SCF_SUBTYPE; adjust PKJK to match

975305e

Disable SCF_SUBTYPE for SAD guess

0b332b6

Rework force-disable of SCF_SUBTYPE with OptionsState

ae51966

Fix up OptionsState

d135f61

Update psi4/src/core.cc

72a0a2d

Co-authored-by: Lori A. Burns <[email protected]>

Update psi4/src/read_options.cc

9f7ddc6

Co-authored-by: Lori A. Burns <[email protected]>

Address more of Lori's suggestions

00e4f76

Retab; spacing should be better?

d5b8dc0

Update read_options.cc

88f9a22

davpoolechem force-pushed the dpoole34/jk-io-control-pk-squash branch from bcc517c to 88f9a22 Compare April 28, 2023 18:35

davpoolechem enabled auto-merge April 28, 2023 20:08

davpoolechem added this pull request to the merge queue Apr 28, 2023

Merged via the queue into psi4:master with commit d2518f2 Apr 28, 2023

DFJK Expert I/O Control Bonus: PKJK #2926

DFJK Expert I/O Control Bonus: PKJK #2926

Uh oh!

Conversation

davpoolechem commented Apr 21, 2023

Description

User API & Changelog headlines

Dev notes & details

Questions

Checklist

Status

Uh oh!

loriab commented Apr 24, 2023

Uh oh!

davpoolechem commented Apr 24, 2023

Uh oh!

loriab commented Apr 24, 2023

Uh oh!

davpoolechem commented Apr 24, 2023

Uh oh!

davpoolechem commented Apr 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loriab Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davpoolechem Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davpoolechem commented Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loriab commented Apr 26, 2023

Uh oh!

davpoolechem commented Apr 26, 2023

Uh oh!

davpoolechem commented Apr 26, 2023

Uh oh!

loriab commented Apr 26, 2023

Uh oh!

davpoolechem commented Apr 26, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

loriab left a comment

Choose a reason for hiding this comment

Uh oh!

davpoolechem commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loriab commented Apr 28, 2023

Uh oh!

davpoolechem commented Apr 28, 2023

Uh oh!

davpoolechem commented Apr 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

loriab Apr 25, 2023 •

edited

Loading

davpoolechem Apr 25, 2023 •

edited

Loading

davpoolechem commented Apr 26, 2023 •

edited

Loading

davpoolechem commented Apr 28, 2023 •

edited

Loading