-
Notifications
You must be signed in to change notification settings - Fork 141
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
Evaluate parameter derivatives for NLPP #4402
Conversation
Add evaluateDerivativesWF and evaluateDerivRatios to RotatedSPOs
The original particleset and the reference particle exist as |
Use the reference particle set and the reference particle index rather than adding them as function arguments.
Add evaluateDerivativesWF to DiracDeterminantBatched
Add evaluateDerivRatios to DiracDeterminantBatched
Test this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to think about expanding coverage. But I would recommend merging the current PR as it is. LGTM.
@@ -206,6 +214,18 @@ class SPOSet : public QMCTraits | |||
const ValueVector& psiinv, | |||
std::vector<ValueType>& ratios); | |||
|
|||
|
|||
/// Determinant ratios and parameter derivatives of the wavefunction for virtual moves | |||
virtual void evaluateDerivRatios(const VirtualParticleSet& VP, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For both added APIs, I feel it will be better to pass the inverse matrix in instead of recomputing inside. Consider that in a later PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, as per my comment above.
for (int j = 0; j < nel; j++) | ||
psiM_inv(i, j) = psiM_all(i, j); | ||
|
||
Invert(psiM_inv.data(), nel, nel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noting the full cubic cost of matrix inversion here. However, getting something working first takes precedence over possible alternatives. Plus considering that the initial electron counts will be smallish, this might not be so bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks Mark! Exciting development.
Test this please |
for (int j = 0; j < nel; j++) | ||
psiM_inv(i, j) = psiM_all(i, j); | ||
|
||
Invert(psiM_inv.data(), nel, nel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the inverse that is likely to get expensive, as it gets call nknot times.
Maybe combining a Sherman-Morrison update with the matrix multiply below will lead to some simplifications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the math can be formulated without any update.
Add
evaluateDerivativesWF
for single determinants to RotatedSPOs. This enables the Hamiltonian parameter derivative from NLPP for single determinant for non-batched NLPP. The function is a reduced version ofevaluateDerivatives
. It only needs the derivative of the wavefunction.Add
evaluateDerivRatios
for single determinants to RotatedSPOs. This enables the Hamiltonian parameter derivative from NLPP for single determinants for the batched NLPP algorithm. The function combines functionality fromevaluateDetRatios
andevaluateDerivativesWF
. Two parameters were added toevaluateDerivRatios
- the original particle set, and the index of the electron being moved. The derivatives of the rotation matrix need an inverse of the entire matrix of orbitals. I don't know if there are shortcuts to get away without needing these, but this is the most straightforward combining of existing code.These changes were pulled out of #4351
What type(s) of changes does this code introduce?
Delete the items that do not apply
Does this introduce a breaking change?
What systems has this change been tested on?
laptop
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.