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.

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

A/E DIIS #2320

Merged
merged 10 commits into from
Feb 10, 2022
Merged

A/E DIIS #2320

merged 10 commits into from
Feb 10, 2022

Conversation

JonathonMisiewicz
Copy link
Contributor

@JonathonMisiewicz JonathonMisiewicz commented Oct 12, 2021 •
edited

Description

Will close #2235 by implementing ADIIS and EDIIS.

I'm submitting this in a draft state to get second opinions on what I have so far. Unfortunately, due to issues with the Francesco group cluster, I can't do deep Vtune profiling, so it may be some time before I can proceed to the next step. I'll keep devs updated.

RHF and UHF EDIIS/ADIIS are working great, but not so much for ROHF. Disappointing, but not surprising. The EDIIS and ADIIS functionals don't account for the effective Fock matrix of ROHF theory.

Todos

  • RHF/UHF implementation
  • Code cleanup
  • Documentation
  • Write tests

Checklist

Status

  • Ready for review
  • Ready for merge

@JonathonMisiewicz JonathonMisiewicz added scf Involves general SCF: convergence algorithms, RHF/UHF/ROHF/CUHF... feature Extends an existing Psi feature or develops a new one. labels Oct 12, 2021
@JonathonMisiewicz JonathonMisiewicz added this to the Psi4 1.5 milestone Oct 12, 2021
@JonathonMisiewicz JonathonMisiewicz changed the title Initial A/E DIIS A/E DIIS Oct 12, 2021
@JonathonMisiewicz
Copy link
Contributor Author

libdiis needs more changes to get it talking with [A/E]DIIS, so it'll be a while before further updates.

@JonathonMisiewicz JonathonMisiewicz modified the milestones: Psi4 1.5, Psi4 1.6 Nov 17, 2021
@JonathonMisiewicz
Copy link
Contributor Author

There will be a change to the next version of the PR: ROHF ADIIS/EDIIS "capabilities" will be removed. As best as I can tell, an ROHF adaptation for ADIIS and EDIIS was never published, my numerical results when I try an adaptation are lackluster, and the formulation is awkward when I try to couple regular DIIS with ADIIS and EDIIS.

If anybody thinks they know how to make ADIIS/EDIIS work for ROHF, let me know. Otherwise, the next version of the PR is RHF/UHF/CUHF only.

@JonathonMisiewicz
Copy link
Contributor Author

During the course of working on this, I've decided a more extensive, very-API breaking refactor will be needed. The problem is that in the current iteration of the code, DIIS has the responsibility for doing the extrapolation, and that naturally fails if the user requests EDIIS but not DIIS.

I'll need to refactor this to have classes just to compute EDIIS/ADIIS/DIIS coefficients, and another class to do the actual extrapolation step.

@JonathonMisiewicz JonathonMisiewicz marked this pull request as ready for review January 13, 2022 04:49
@JonathonMisiewicz
Copy link
Contributor Author

This should be a functional PR. Adding tests would still be good, but I'll gladly take suggestions on what this tests should be.

@JonathonMisiewicz
Copy link
Contributor Author

And I suppose now there's a dependency question: how do we feel about making scipy a dependency? That's why tests fail on test environment but full tests passed locally.

@susilehtola
Copy link
Member

This should be a functional PR. Adding tests would still be good, but I'll gladly take suggestions on what this tests should be.

How about the cadmium complex from Hu&Yang, and UF4 from Kudin et al? These probably don't converge without this branch.

@JonathonMisiewicz
Copy link
Contributor Author

This should be a functional PR. Adding tests would still be good, but I'll gladly take suggestions on what this tests should be.

How about the cadmium complex from Hu&Yang, and UF4 from Kudin et al? These probably don't converge without this branch.

Added and passing, thanks.

maxscheurer
maxscheurer reviewed Jan 18, 2022
View reviewed changes
Copy link
Contributor

@maxscheurer maxscheurer left a comment

Choose a reason for hiding this comment

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

First pass done 👍 This is an awesome feature addition! 🚀

psi4/driver/procrouting/diis.py Outdated Show resolved Hide resolved
psi4/driver/procrouting/diis.py Outdated Show resolved Hide resolved
psi4/driver/procrouting/diis.py Outdated Show resolved Hide resolved
psi4/driver/procrouting/diis.py Show resolved Hide resolved
psi4/driver/procrouting/diis.py Outdated Show resolved Hide resolved
tests/aediis-1/input.dat Outdated Show resolved Hide resolved
psi4/driver/procrouting/diis.py Outdated Show resolved Hide resolved
psi4/driver/procrouting/diis.py Outdated Show resolved Hide resolved
psi4/driver/procrouting/scf_proc/subclass_methods.py Show resolved Hide resolved
jturney
jturney approved these changes Feb 3, 2022
View reviewed changes
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.

Looks very nice and clean!

susilehtola
susilehtola approved these changes Feb 3, 2022
View reviewed changes
Copy link
Member

@susilehtola susilehtola left a comment

Choose a reason for hiding this comment

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

Other than the keyword naming looks good

hokru
hokru reviewed Feb 4, 2022
View reviewed changes
doc/sphinxman/source/scf.rst Outdated Show resolved Hide resolved
maxscheurer
maxscheurer approved these changes Feb 7, 2022
View reviewed changes
Copy link
Contributor

@maxscheurer maxscheurer left a comment

Choose a reason for hiding this comment

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

LGTM 👍 I have no strong opinion on the INITIAL_... keyword names, I think the current version, given the documentation, is fine. So it's up to you and/or others to decide whether to change these names or not 😄

loriab
loriab approved these changes Feb 7, 2022
View reviewed changes
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! only a few suggestions.

doc/sphinxman/source/scf.rst Outdated Show resolved Hide resolved
psi4/driver/procrouting/scf_proc/scf_iterator.py Outdated Show resolved Hide resolved
psi4/src/read_options.cc Outdated Show resolved Hide resolved
tests/aediis-1/input.dat Outdated Show resolved Hide resolved
tests/aediis-2/input.dat Outdated Show resolved Hide resolved
@JonathonMisiewicz
Copy link
Contributor Author

The new keywords are

SCF_INITIAL_ACCELERATOR
SCF_INITIAL_START_DIIS_TRANSITION
SCF_INITIAL_FINISH_DIIS_TRANSITION

If anybody is unhappy with keyword names, provide an alternative name. Otherwise, I'll be merging this in 72 hours.

@JonathonMisiewicz JonathonMisiewicz merged commit be533f7 into psi4:master Feb 10, 2022
zachglick pushed a commit to zachglick/psi4 that referenced this pull request Apr 18, 2022
@JonathonMisiewicz @zachglick
A/E DIIS (psi4#2320)
8e5a146 
* A/EDIIS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hokru hokru hokru approved these changes

@jturney jturney jturney approved these changes

@susilehtola susilehtola susilehtola approved these changes

@loriab loriab loriab approved these changes

@maxscheurer maxscheurer maxscheurer approved these changes

Assignees
No one assigned
Labels
feature Extends an existing Psi feature or develops a new one. scf Involves general SCF: convergence algorithms, RHF/UHF/ROHF/CUHF...
Projects
None yet
Milestone
Psi4 1.6
Development

Successfully merging this pull request may close these issues.

Implement EDIIS
6 participants
@JonathonMisiewicz @susilehtola @jturney @hokru @loriab @maxscheurer