QMCPACK
[WIP] Prototype for orbital rotation using global rotation
This implements option 3 in https://github.com/QMCPACK/qmcpack/issues/3983#issuecomment-1197380631 It maintains global rotation parameters instead of a history list.
The call to resetParameters does
- Compute the change in parameters from the old ones in myVars and the new ones in 'active'.
- Compute rotation matrices corresponding the old and delta parameters
- Multiply rotation matrices
- Apply rotation to stored MO coefficients
- Extract the rotation parameters from the new rotation matrix and set those the new parameters.
One subtle point is we need to track all the rotation parameters, not just the subset of parameters being optimized.
This code does store a copy of the coefficient matrix to compute rotated parameters, so it will only work with LCAO.
This uses the same infrastructure to save and restore the extra parameter info as in #4146.
The PR is not intended to be checked in. The intent to make this option visible and available for testing.
Proposed changes
Describe what this PR changes and why. If it closes an issue, link to it here with a supported keyword.
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.
- No. 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
- 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)
"This code does store a copy of the coefficient matrix to compute rotated parameters, so it will only work with LCAO." This is a bit worrisome as the code that @jptowns has worked with can be applied to splines as well. What is the plan to generalize this?
For an SPOSet to work with this implementation of the feature, it needs to implement storeParamsBeforeRotation and follow the "use_stored_copy" flag to applyRotation. (Not following the "use_stored_copy" flag is the same as setting it to "false")
The stored-history implementation in #4146 doesn't need storeParamsBeforeRotation, and has "use_stored_copy" always set to "false". This global rotation implementation has "use_stored_copy" always set to "true".
This may be one reason to favor the stored-history implementation - it requires fewer changes for the underlying SPOSet.
I think the current spline implementation effectively uses the stored-history implementation. Though it seems like it should have problems when running threaded, but I haven't looked at it in detail. (The problem should come when computing the rotation in resetParameters - checkInVariables on RotatedSPOs is only called on one thread)
@markdewing please can you sync with @jptowns and put plan forward here? The question is about the preferred implementation going forward. We need spline rotations to work as well as SPOs with urgency. Ideally we would only have one implementation of how to do this for simplicity - it that possible, at least for (say) running FeO bulk?
I feel like this option still leaves the concurrent operation of the rotational optimization very unclear IMHO. And the root cause of this is because of the index = 0 of a walker scope object is magic QMCPACK design pattern.
Regardless of which way of doing this is chosen. The SPOSets are not the single particle orbitals. They are a bunch of walker scope result caching and evaluation functions with a shared pointer to the rank scope data structure that is the actual SPO which is the coefficients and the basis set over which they generate the single particle orbitals.
The method of creating and updating this object (the actual SPO) should not be and never should have been through the SPOSet API. It should either be an argument to every evaluate method in the SPOSet API or to the SPOSet constructor. It should not be a shared_ptr since it can have a clear lifetime enclosing the SPOSet lifetimes now.
Then the problem of any rotation call needing to only be called by one instance in one thread or produce the same result if called multiple times (even worse since this requires a mutex) disappears. Since it appears explicitly as the rank scope object it is you have to call it from the rank concurrency context. It seems like the history or series of rotation matrices or whatever is at least in part also a rank scope API and would make more sense at that scope. The walker scope API should involve only what work is done at the walker level.
"This code does store a copy of the coefficient matrix to compute rotated parameters, so it will only work with LCAO." This is a bit worrisome as the code that @jptowns has worked with can be applied to splines as well. What is the plan to generalize this?
When apply the rotation, the matrix multiplication needs to be out-of-place. So we have both the old (in) coefficients (spline/MO) and new(out) coefficients. LCAO calls copy of old coefficients in checkInVaraibles and then used it in applyRotation. SplineR2R makes the copy directly in the applyRotation.
With @markdewing 's change relying on original coefficients, this original coefficients set must be stored somewhere either inside the SPOSet object or as a separate SPOSet object. We right now are doing the former. This means that SplineR2R needs a bit change to store the original spline table. We might change to the latter option but we need to do things in separate stages.
Once we keep the original coefficients in a separate named SPOSet, there is no need of internal copy of coefficients. Both original and rotated SPOSet can function by themselves and this opens doors to many use cases. SPOSet is polymorphic and thus the way to rotate should be implemented inside the SPOSet and thus via SPOSet API. The intention of shared_ptr is that all the SPOSet object with the same name should share the same pointer.
In my other WFOpt related changes, all the WFopt parameter related APIs will be called outside the TWF tree. All the OptimizableOjbect will be extracted from the gold TWF and then being called outside the TWF tree. Calling update once and all the TWF clones receiving the change right away is the design goal. There is no need to make applyRotation working with multiple threads. It is strictly disallowed. In a RHF calculation, applyRotation also should only be called once, that is another reason of calling parameter update outside the TWF tree.
