Update OptimizableObject and its usage #4168
Conversation
|
@PDoakORNL in this PR OptimizableObject is moved out of base classes and added as a base class only on their derived classes with actual optimizable parameters. |
|
Is there a plan to get rid of checkInVariables and resetParameters in favor of using only checkInVariablesExclusive and resetParametersExclusive? (In other words, is having both functions in the API an intermediate state of the code?) |
|
Eventually I would like an API that removes the need to call checkOutVariables to propagate the global-to-local index mapping. |
Yes. SPO/WFC level of checkInVariables and resetParameters will be removed. Not there yet ;) |
I found checkOutVariables is still needed, it settings internal state for later evaluateDerivatives. In case of ROHF, there is only one set of sposet but there are two objects. So checkOutVariables needs to be called on both objects and thus needs to be called from TWF tree. |
| * }; | ||
| * A vector will be created by calling extractOptimizableObjects(). | ||
| * All the checkInVariables() will be called through this vector and thus | ||
| * checkInVariablesExclusive implementation should only handle non-OptimizableObject members. |
There was a problem hiding this comment.
So to me the use of inclusive and exclusive here is not very clear. I immediately thought of it terms of thread when I saw the Exclusive on this method. i.e. only call if you are certain you will be the only caller. Exclusive/Inclusive I might also have the sense of a range that does or doesn't include the endpoints or an operation that checks in any variable not already checked in (XOR/OR).
The difference here is whether the call traverses the implicit tree of optimizables below the object called with checkInVariables or not. So its easier for me to think of it as a "Traversing" checkInVariables (the legacy call) or a NonTraversing the new checkInVariablesExclusive call. Or as a recursive vs. nonrecursive call.
There was a problem hiding this comment.
It does seem clear in the new design the traversal occurs with extractOptimizableObjects so the checkInVariables must be non traversing.
I don't think this is a bad design especially since the "tree" is adhoc and each "node" used to class handle calling its leaves/branches in an adhoc way.
Now that "tree" traversal is in classes extract method and the checkIn/reset/whatever are in methods that just do that which is better.
There was a problem hiding this comment.
I was taking the notion from inclusive/exclusive timer.
| void resetParameters(const opt_variables_type& active); | ||
| void reportStatus(std::ostream& os) final; | ||
|
|
||
| // extractOptimizableObjectRefs is not enabled in BackflowTransformation. |
There was a problem hiding this comment.
I assume this is part of why this object is final? Not sure whether this needs to be documented more at this time but it leaves me wondering why BackFlowTransformation is a special case. This probably really amounts to BackFlow needing better documentation which is beyond this PR.
There was a problem hiding this comment.
Right now there is a Clang compiler warning. Adding final in the class stops the warning. BackFlowTransformation should never be inherited and thus I think adding final is OK.
|
@markdewing Does this PR look good to you? |
|
Test this please |
Proposed changes
Add OptimizableObject as a base class for classes directly owning variational parameters.
No more as base class of WFC/SPOSet.
Following this API design.
https://github.com/ye-luo/qmcpack/wiki/Optimization-API-changes
What type(s) of changes does this code introduce?
Does this introduce a breaking change?
What systems has this change been tested on?
epyc-server
Checklist