Skip to content

Fixes orbital read for finite difference hessian #2827

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
JonathonMisiewicz merged 3 commits into from
Dec 14, 2022
Merged

Fixes orbital read for finite difference hessian #2827

JonathonMisiewicz merged 3 commits into from
Dec 14, 2022

Conversation

@AlexHeide
Copy link
Contributor

@AlexHeide AlexHeide commented Dec 6, 2022 •
edited
Loading

Description

Fixes a bug encountered when running optimizations in BAKERJCC96. All calculations in a finite difference
calculation were writing orbitals to disk and overwriting the old orbitals. Only reference calculation should
be saved. In cases were the symmetry was lowered in one of the last displacements, subsequent gradient calculations
were failing to read orbitals.

File "/home/zander/github/psi4/objdir/stage/lib/psi4/driver/procrouting/proc.py", line 1761, in scf_helper
    raise ValidationError("Cannot compute projection of different symmetries.")

User API & Changelog headlines

Dev notes & details

  • scf_helper assumes orbitals should always be written. Passes write_orbitals = false through kwargs for displacements

Questions

  • The new test is an optimization, would a test in ddd-function-kwargs or similar be preferred?

Checklist

  • Tests added for any new features
  • full ctest (Psi4, Psi4 + [CheMPS2, DFTD3, dkh, gCP, gdma, simint, ecpint])

Status

  • Ready for review
  • Ready for merge

@loriab
Copy link
Member

loriab commented Dec 6, 2022

are the new tests hitting the bug? code change was to findif. if it's not too long, you could have a dertype=1 and a dertype=0 in the test input.

@AlexHeide
Copy link
Contributor Author

AlexHeide commented Dec 6, 2022

Yes, the new test would hit the bug and raise the ValidationError above in 1.6 and 1.7. The test is very quick.

@loriab loriab added this to the Psi4 1.8 milestone Dec 7, 2022
loriab
loriab approved these changes Dec 12, 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, thanks!

for any others who are wondering how write_orbitals is getting turned on, I think it's here

JonathonMisiewicz
JonathonMisiewicz reviewed Dec 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.

The code looks good, and the test case is good.

For posterity, please update the test intro to explicitly say that optimization will read the previous set of orbitals.

JonathonMisiewicz
JonathonMisiewicz approved these changes Dec 13, 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!

@JonathonMisiewicz
Copy link
Contributor

JonathonMisiewicz commented Dec 14, 2022

@AlexHeide Notify us when this PR can be merged. You haven't checked the "ready to merge" box yet.

@AlexHeide
Copy link
Contributor Author

AlexHeide commented Dec 14, 2022

rdy!

@JonathonMisiewicz JonathonMisiewicz merged commit 4d94910 into psi4:master Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@loriab loriab loriab approved these changes

@JonathonMisiewicz JonathonMisiewicz JonathonMisiewicz approved these changes

Assignees

No one assigned

Labels

bug optking

Projects

None yet

Milestone

Psi4 1.8

Development

Successfully merging this pull request may close these issues.

3 participants

@AlexHeide @loriab @JonathonMisiewicz