QMCPACK
Support multi step WFopt
Review after #4168
Proposed changes
Introduce multi-step WFopt, J2, J12, J12msd
<qmc method="linear">
<optimized_subset> uu ud </optimized_subset>
</qmc>
<qmc method="linear">
<optimized_subset> eH uu ud </optimized_subset>
</qmc>
<qmc method="linear">
<optimized_subset> eH uu ud msd_coefs </optimized_subset>
</qmc>
What type(s) of changes does this code introduce?
- New feature
Does this introduce a breaking change?
- No
What systems has this change been tested on?
epyc-server
Checklist
- Yes. This PR is up to date with current the current state of 'develop'
Many of the optimizable wavefunction fragments have an "optimize" attribute in the input. How does this interact with that attribute?
Many of the optimizable wavefunction fragments have an "optimize" attribute in the input. How does this interact with that attribute?
Good question. My here is what I found. The optimize=yes/no still works. It is a predominant switch. If it is yes, then that component can be selected at WFOpt.
However, "optimize" control default depends on each class. MSD coefficient default to no. Jastrow coefficients default to yes. I can understand why such default values are choose, on the other hand, I'm wondering if we still need this two level control. I'm in favor of removing the control in WF and use WFOpt.
I think that specifying which individual parts of a wavefunction should be kept fixed/frozen and which are to be optimized is naturally done at the wf component level, since the flag is immediately adjacent to the parameters, can be loaded/saved with them, and an extra step of indirection/naming is avoided. e.g. Optimize one and two body jastrows, add a three body keeping the others fixed etc.. This would be an argument for supporting both provided the complexities are not too high, and would also make using the new scheme optional for existing users. In the future, once the new scheme is proven we could consider dropping the old one.
Since QMCPACK can support multiple optimization runs from a single input, I do think that this is a good idea to include.
Please add tests where components of the wavefunction that do not exist are mentioned (typos, incorrect input recycling). The code should error out and give an appropriate message.
I won't disagree backward compatibility. That is why the existing "optimize" in WF are untouched. "which are to be optimized is naturally done at the wf component level" I don't disagree.
However there are issues around this choice.
-
<msd optimize="no"/>Does this mean the CI coefs fixed or the whole msd is fixed including orbitals? The new way goes with coefficient sets instead of components. There is no such ambiguity. - optimized or not is an info only necessary during WFOpt. So providing such info at the driver is neat.
- simplify the workflow by avoid touching wf blocks or files.
Right now the old method (predominant) and the new method (secondary) do work together.
Ye: I agree, all good points.
Does this mean the CI coefs fixed or the whole msd is fixed including orbitals?
I assume that we'll support any combination: ci coefs, orbital coefs, basis set coefs, jastrow N coefs, backflow coefs etc. At least for optimization testing, and likely in practice, all combinations are required.
I like this as a per-optimizer block feature. The only change/request I would make is to not innovate on adding lots of new xml tags. Almost all other inputs to optimizers (and the rest of the drivers) are handled via parameters:
<qmc method="linear">
<parameter name="optimized_subset"> eH uu ud msd_coefs </parameter>
</qmc>
One issue is distinguishing between variational parameters that should be read in from a previous optimization run but kept fixed, and ones that should be optimized during the current run. Putting the list of wavefunction parts in the driver makes this distinction clearer. Orbital rotation is an extreme example - currently the class isn't even created unless optimize is set to true. I don't think it's even possible to read in rotation parameters and not optimize them. (Ye has a suggestion elsewhere to change how orbital rotation is specified in the input file which would fix this.).
The vp.h5 files don't support reading parameters and keeping them fixed in an optimizer run. This is something that will need to be addressed in the future.
A small comment on the name - "optimized" is past tense, but the intent is to provide a list of parts to optimize in the current run. "optimizable_subset"? - more accurate, but a little wordier. And "optimizable" still makes it sound like something that has the potential to be optimized, not that it necessarily is being optimized. "optimizing_subset"? (I don't really like that, either)
Another question is: subset of what? This will definitely need to be answered in the docs, but is there a way to make it clearer in the name?
This relates to another question about the use of "Object" in OptimizableObject. It's such a generic name - is there something more descriptive that could be used? (There are many other terms, but they're all still pretty generic). "coefficient set" was mentioned, and I like that.
Maybe the parameter could be called "optimizable_coefficient_sets" or "optimizing_coefficient_sets"? Or "active_optimizable_coefficient_sets"? (getting a little long, though)
I like this as a per-optimizer block feature. The only change/request I would make is to not innovate on adding lots of new xml tags. Almost all other inputs to optimizers (and the rest of the drivers) are handled via parameters:
<qmc method="linear"> <parameter name="optimized_subset"> eH uu ud msd_coefs </parameter> </qmc>
I tried this way and hit the limitation of parameter input handling which doesn't support a list of strings.
@ye-luo it does for 1RDM:
<estimator type="dm1b" name="DensityMatrices">
<parameter name="basis" > spo_u spo_uv </parameter>
<parameter name="evaluator" > matrix </parameter>
<parameter name="integrator" > uniform_grid </parameter>
<parameter name="points" > 4 </parameter>
<parameter name="scale" > 1.0 </parameter>
<parameter name="center" > 0 0 0 </parameter>
</estimator>
I suggest we update parameter handling (if needed) to handle lists of the basic types. This is much better than proliferating tags with potentially arbitrarily formatted text blocks inside as it is in the direction of uniform input handling.
I also wish to see the parameter route (only) supported since it makes for a more consistent input experience and means there is less to remember and less for other tools to implement. Support should be trivial - e.g. we can just treat this a single big string for now. No need for fancy list handling. Keep things simple.
@ye-luo it does for 1RDM:
<estimator type="dm1b" name="DensityMatrices"> <parameter name="basis" > spo_u spo_uv </parameter> <parameter name="evaluator" > matrix </parameter> <parameter name="integrator" > uniform_grid </parameter> <parameter name="points" > 4 </parameter> <parameter name="scale" > 1.0 </parameter> <parameter name="center" > 0 0 0 </parameter> </estimator>I suggest we update parameter handling (if needed) to handle lists of the basic types. This is much better than proliferating tags with potentially arbitrarily formatted text blocks inside as it is in the direction of uniform input handling.
This was working because it bypasses ParameterSet and directly handle the XML. We should avoid this.
I found a way to first load multiple value as a string and then convert the string to a vector of something.
Thanks for editing the topmost entry to reflect the final input style:
<qmc method="linear"> <parameter name="optimized_subset"> eH uu ud msd_coefs </parameter> </qmc>I haven't had a chance to test this: what currently happens when varaiaional_subset includes a parameter set that does not exist? (typo etc.)
good catch. I updated the code to trap invalid entries.
Can you please add a test of the failure case? There are other PRs cooking, so it won't slow this one. I think it is good policy to add these checks where we can reasonably do so.