Skip to content

tag reminder and cbs bug fix #2590

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
loriab merged 1 commit into from
Jun 7, 2022
Merged

tag reminder and cbs bug fix #2590

loriab merged 1 commit into from
Jun 7, 2022

Conversation

@loriab
Copy link
Member

@loriab loriab commented May 27, 2022 •
edited
Loading

Description

Todos

  • If you build from source, and your tags aren't synced with upstream, the Psi4 version will be undefined. QCEngine balks at that and urges you to pull tags. But this only pops up for cbs/findif/nbody jobs, which can be confusing. This PR promotes to tag complaint to upon import psi4 for uniformity.
  • "OliP" reported a bug at the forum where an input like the below returned ‘<’ not supported between instances of ‘dict’ and ‘dict’ . This came about because I was comparing well-behaved dertype per method tuples like
['hf', 'mp2', 'ccsd(t)', 'ccsd(t)']
[(2, {}), (1, {}), (1, {}), (1, {})]

rather than the real-life cases one gets with ROHF. Sorting on only the dertype fixes the bug.

['hf', 'mp2', 'ccsd(t)', 'ccsd(t)']
[(2, {}), (0, {1: "\nPsiException: select_mp2_gradient: Method 'mp2' with MP2_TYPE 'DF', FREEZE_CORE 'True', and REFERENCE 'ROHF' not available\n\n"}), (0, {1: "\nPsiException: select_ccsd_t__gradient: Method 'ccsd(t)' with CC_TYPE 'CONV' and REFERENCE 'ROHF' not available\n\n"}), (0, {1: "\nPsiException: select_ccsd_t__gradient: Method 'ccsd(t)' with CC_TYPE 'CONV' and REFERENCE 'ROHF' not available\n\n"})]

molecule N {
0 4
N 0.00 0.00 0.00
}

set {
scf_type direct
reference rohf
r_convergence 6
d_convergence 7
e_convergence 8
freeze_core true
}

e_cbs = energy(‘cbs’,
scf_basis=‘aug-cc-pV[TQ5]Z’,
corl_wfn=‘mp2’,
corl_basis=‘aug-cc-pV[TQ]Z’,
delta_wfn=‘ccsd(t)’,
delta_basis=‘aug-cc-pV[DT]Z’)
  • @hokru reported that energy('SCF/def2-SVP',bsse_type=['cp','nocp'],dft_functional=my_functional) got broken with the distributed driver. That's patched up for string dft_functional at least.
  • I noticed that a change I made to _qcvars_transitions to help version reported would make variables(include_deprecated_keys=True) fail. that's fixed.

Checklist

Status

  • Ready for review
  • Ready for merge

@JonathonMisiewicz
Copy link
Contributor

JonathonMisiewicz commented May 27, 2022

  1. Please add a test showing the bug is fixed.
  2. Please explain the bug in more detail. Is the expected behavior that Psi reports some jobs in the plan can't run? If the expected behavior is that you get a number, why are any PsiExceptions appearing in the plan?

@loriab
Copy link
Member Author

loriab commented Jun 2, 2022

Please explain the bug in more detail. Is the expected behavior that Psi reports some jobs in the plan can't run? If the expected behavior is that you get a number, why are any PsiExceptions appearing in the plan?

Good point. The new driver_util tech separates figuring out what derivative psi can do analytically under the specified conditions from what derivative requested or demanded by user. So those error strings you see in analytic are just being stored in case user demanded analytic gradient. In the given case, energy was requested and the error strings were never used. If the user wanted gradient, calc would fall back to findif by E and the error strings wouldn't be used. If the user wanted gradient and also gave dertype=1 for analytic, the error string pops up.

For the last case, this needs to be improved in future to show the user all the errors, not just the first (mp2) one. But that's more than a couple-line fix, so not suitable for backport.

@loriab loriab added this to the Psi4 1.7 milestone Jun 3, 2022
zachglick
zachglick approved these changes Jun 5, 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.

Thanks for the detailed explanation!

@loriab loriab merged commit 22a88b9 into master Jun 7, 2022
@loriab loriab deleted the loriab-patch-1 branch June 7, 2022 18:20
loriab added a commit that referenced this pull request Jul 6, 2022
@loriab
ddd bugfixes (#2590)
bcd0eca
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PeterKraus PeterKraus PeterKraus approved these changes

@zachglick zachglick zachglick approved these changes

@JonathonMisiewicz JonathonMisiewicz JonathonMisiewicz approved these changes

Assignees

No one assigned

Labels

backport

Projects

None yet

Milestone

Psi4 1.7

Development

Successfully merging this pull request may close these issues.

5 participants

@loriab @JonathonMisiewicz @PeterKraus @zachglick