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.

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

Fix Incorrect Expressions for Hamiltonian and Overlap Matrices with Complex Wavefunctions #4821

Merged
merged 6 commits into from
Nov 8, 2023

Conversation

rcclay
Copy link
Contributor

@rcclay rcclay commented Nov 8, 2023

Please review the developer documentation
on the wiki of this project that contains help and requirements.

Proposed changes

For the linear method, mainline QMCPACK builds the Umrigar/Toulouse Hamiltonian and Overlap matrices from d logpsi/dc, E_L, and dE_L/dc, where derivatives are taken w.r.t. variational parameters c. Before the code actually builds the matrices, it takes the real part of all components. For real wave functions, this is fine, but is obviously incorrect for complex wave functions, the first smoking gun being the lack of complex conjugation in the wave function overlap matrices.

For what follows notationally, refer to: https://pubs.aip.org/aip/jcp/article/126/8/084102/309065

If one rederives the Umrigar/Toulouse 2007 expressions under the assumptions of:
1.) Complex wave function
2.) Real local energy
3.) Real variational parameters (enforced by PR#4819, which implies real dE_L/dc derivatives),

We arrive at the following expressions for the Hamiltonian and Overlap matrices respectively.
h_ij
overlap

This PR implements the above expressions by changing Dsaved type from Real to ValueType, adding in appropriate complex conjugation, and deferring taking the real part until we finally build the Hamiltonian and overlap matrices.

What type(s) of changes does this code introduce?

Delete the items that do not apply

  • Bugfix

Does this introduce a breaking change?

  • No

What systems has this change been tested on?

Intel Xeon, RHEL8, Gnu compilers.

Checklist

Update the following with a yes where the items apply. If you're unsure about any of them, don't hesitate to ask. This is
simply a reminder of what we are going to look for before merging your code.

  • Yes. This PR is up to date with current the current state of 'develop'
  • No. Code added or changed in the PR has been clang-formatted
  • No. This PR adds tests to cover any new code, or to catch a bug that is being fixed
  • No. Documentation has been added (if appropriate)

@rcclay rcclay requested review from ye-luo and markdewing November 8, 2023 00:23
@ye-luo
Copy link
Contributor

ye-luo commented Nov 8, 2023

How hard it is to fix the non-batched code path?

@rcclay
Copy link
Contributor Author

rcclay commented Nov 8, 2023

As easy as remembering to git commit.

@rcclay
Copy link
Contributor Author

rcclay commented Nov 8, 2023

There's a nonzero chance that this will trash whatever complex optimization integration tests we have.

Copy link
Contributor

@ye-luo ye-luo left a comment

Choose a reason for hiding this comment

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

If no other comments come, will merge tomorrow.

@ye-luo
Copy link
Contributor

ye-luo commented Nov 8, 2023

Test this please

@prckent prckent self-requested a review November 8, 2023 14:35
Copy link
Contributor

@prckent prckent 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 working through this. Apparently we will need tougher integration tests of complex optimization :-) It would be good to put more of the references and links to analysis in the source code so that we can find the formal formulas in future.

@rcclay
Copy link
Contributor Author

rcclay commented Nov 8, 2023

deterministic-unit_test_new_driver failure in mixed precision complex builds. I'll take a look.

@ye-luo
Copy link
Contributor

ye-luo commented Nov 8, 2023

deterministic-unit_test_new_driver failure in mixed precision complex builds. I'll take a look.

I have a fix for this and will PR separately.

@rcclay
Copy link
Contributor Author

rcclay commented Nov 8, 2023

Thank you!!!

@ye-luo
Copy link
Contributor

ye-luo commented Nov 8, 2023

Test this please

@ye-luo ye-luo enabled auto-merge November 8, 2023 21:38
@ye-luo ye-luo merged commit dd121a2 into QMCPACK:develop Nov 8, 2023
23 checks passed
@rcclay rcclay deleted the complex_opt_fix branch November 22, 2023 16:04
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.

3 participants