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

Evaluate parameter derivatives for NLPP #4402

Merged
merged 10 commits into from
Jan 26, 2023
Merged

Conversation

markdewing
Copy link
Contributor

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 of evaluateDerivatives. 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 from evaluateDetRatios and evaluateDerivativesWF. Two parameters were added to evaluateDerivRatios - 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

  • New feature

Does this introduce a breaking change?

  • No

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.

  • Yes This PR is up to date with current the current state of 'develop'
  • Yes Code added or changed in the PR has been clang-formatted
  • Yes. 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)

@ye-luo
Copy link
Contributor

ye-luo commented Jan 21, 2023

The original particleset and the reference particle exist as VirtualParticleSet::getRefPS() and refPtcl. Preferably refPtcl should be marked private and accessed via an API (A later topic/PR). evaluateDerivRatios can be restored as it was.

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
@markdewing
Copy link
Contributor Author

Test this please

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.

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,
Copy link
Contributor

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.

Copy link
Contributor

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);
Copy link
Contributor

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.

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.

LGTM

Thanks Mark! Exciting development.

@prckent
Copy link
Contributor

prckent commented Jan 26, 2023

Test this please

@ye-luo ye-luo enabled auto-merge January 26, 2023 15:55
@ye-luo ye-luo merged commit 18fefe7 into QMCPACK:develop Jan 26, 2023
for (int j = 0; j < nel; j++)
psiM_inv(i, j) = psiM_all(i, j);

Invert(psiM_inv.data(), nel, nel);
Copy link
Contributor Author

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.

Copy link
Contributor

@ye-luo ye-luo Jan 26, 2023

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.

@markdewing markdewing deleted the eval_derivs branch January 26, 2023 16:39
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.

None yet

3 participants