Skip to content

Conversation

@susilehtola
Copy link
Member

@susilehtola susilehtola commented Jan 19, 2022

Description

The current guess mix code runs in iteration 0, but SAD doesn't have orbitals there, while some other guesses don't call form_C in iteration 0. This PR fixes guess_mix for all guesses.

Todos

  • Tests for guess_mix added for all guesses

Questions

  • Question1

Checklist

Status

  • Ready for review
  • Ready for merge

@JonathonMisiewicz
Copy link
Contributor

JonathonMisiewicz commented Jan 19, 2022

The PR mostly looks good. A few questions:

  1. Can we get an output.ref on the test?
  2. On the linked issue (Wrong guess_mix when guess=sad #2410), you said this is a "tentative" fix. Does that mean anything besides that the PR isn't accepted yet?
  3. I'll hold off on accepting until the "documentation" issue is settled.

@JonathonMisiewicz JonathonMisiewicz linked an issue Jan 19, 2022 that may be closed by this pull request
@susilehtola
Copy link
Member Author

2. On the linked issue ([Wrong guess_mix when guess=sad #2410](https://github.com/psi4/psi4/issues/2410)), you said this is a "tentative" fix. Does that mean anything besides that the PR isn't accepted yet?

It means I was able to quickly identify the issue and throw together a simple fix, but not to test it ;) I still don't have a working Psi4 compile environment due to the libint2 incompatibility. Once that gets fixed, I'll have to push to get newer versions of Psi4 to compile again in Fedora.

@susilehtola
Copy link
Member Author

So... looks like it works; DIIS just takes it to the wrong solution

                          Total Energy        Delta E     RMS |[F,P]|

   @DF-UHF iter SAD:    -0.70872378254081   -7.08724e-01   0.00000e+00 
  Mixing alpha HOMO/LUMO orbitals (1,2)

   @DF-UHF iter   1:    -0.82640393434897   -1.17680e-01   1.12200e-03 DIIS
   @DF-UHF iter   2:    -0.97800267490794   -1.51599e-01   1.18746e-02 DIIS
   @DF-UHF iter   3:    -0.82673929652084    1.51263e-01   9.92192e-04 DIIS
   @DF-UHF iter   4:    -0.82768446534307   -9.45169e-04   2.11271e-03 DIIS
   @DF-UHF iter   5:    -0.82648408142877    1.20038e-03   9.77585e-06 DIIS
   @DF-UHF iter   6:    -0.82648447847074   -3.97042e-07   3.83550e-05 DIIS
   @DF-UHF iter   7:    -0.82648407869393    3.99777e-07   1.24969e-06 DIIS
   @DF-UHF iter   8:    -0.82648407827585    4.18083e-10   7.42903e-08 DIIS
   @DF-UHF iter   9:    -0.82648407827446    1.38667e-12   8.54867e-10 DIIS
   @DF-UHF iter  10:    -0.82648407827446    7.77156e-16   1.13025e-12 DIIS
  Energy and wave function converged.

@susilehtola susilehtola changed the title Fix guess mix with SAD Fix guess mix with SAD and other guesses Jan 19, 2022
@susilehtola
Copy link
Member Author

OK, tests appear to pass now with all guesses.

I can't generate output.ref so I would appreciate if someone can compile the branch locally and contribute the missing files.

@JonathonMisiewicz
Copy link
Contributor

Running the same test but with different options is a perfect reason to write a pytest rather than a C-test, and no output.ref are needed in that case. If you can't even compile Psi, I can write the Pytest myself and submit a PR to your branch, because at least I can test it easily.

Thanks for the debugging thus far!

}

// SAD doesn't have orbitals in iteration 0, other guesses do
bool have_orbitals = !sad_ || (sad_ && iteration_ > 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

In my ideal world, we could just check if Ca_ was nullptr, but getting Psi to such a state is beyond-the-scope of this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup...

@JonathonMisiewicz
Copy link
Contributor

Accidentally pushed rather than made a PR to your fork. Oops. LMK if you object, @susilehtola.

@susilehtola
Copy link
Member Author

Accidentally pushed rather than made a PR to your fork. Oops. LMK if you object, @susilehtola.

that's fine

@jturney jturney merged commit eb5c5af into psi4:master Jan 20, 2022
@susilehtola susilehtola deleted the sad_mix branch January 21, 2022 17:29
@JonathonMisiewicz JonathonMisiewicz added this to the Psi4 1.6 milestone Feb 11, 2022
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.

4 participants