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

Support multi step WFopt #4169

Merged
merged 7 commits into from
Aug 19, 2022
Merged

Conversation

ye-luo
Copy link
Contributor

@ye-luo ye-luo commented Aug 11, 2022

Review after #4168

Proposed changes

Introduce multi-step WFopt, J2, J12, J12msd

<qmc method="linear">
  <parameter name="optimized_subset"> uu ud </parameter>
</qmc>
<qmc method="linear">
  <parameter name="optimized_subset"> eH uu ud </parameter>
</qmc>
<qmc method="linear">
  <parameter name="optimized_subset"> eH uu ud CI </parameter>
</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'

@ye-luo ye-luo marked this pull request as draft August 11, 2022 13:53
@markdewing
Copy link
Contributor

Many of the optimizable wavefunction fragments have an "optimize" attribute in the input. How does this interact with that attribute?

@ye-luo
Copy link
Contributor Author

ye-luo commented Aug 11, 2022

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.

@prckent
Copy link
Contributor

prckent commented Aug 11, 2022

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.

@prckent
Copy link
Contributor

prckent commented Aug 11, 2022

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.

@ye-luo
Copy link
Contributor Author

ye-luo commented Aug 11, 2022

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.

  1. <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.
  2. optimized or not is an info only necessary during WFOpt. So providing such info at the driver is neat.
  3. simplify the workflow by avoid touching wf blocks or files.

Right now the old method (predominant) and the new method (secondary) do work together.

@prckent
Copy link
Contributor

prckent commented Aug 11, 2022

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.

@jtkrogel
Copy link
Contributor

jtkrogel commented Aug 11, 2022

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>

@markdewing
Copy link
Contributor

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.

@markdewing
Copy link
Contributor

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)

@ye-luo
Copy link
Contributor Author

ye-luo commented Aug 11, 2022

How about variational_subset? variational subset of parameters.

@ye-luo
Copy link
Contributor Author

ye-luo commented Aug 11, 2022

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.

@@ -0,0 +1,92 @@
<?xml version="1.0"?>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$ qmca -q ev *.scalar.dat
 
                            LocalEnergy               Variance           ratio 
H4-OneShiftOnly  series 0  -1.904150 +/- 0.004328   0.690708 +/- 0.008875   0.3627 
H4-OneShiftOnly  series 1  -1.993150 +/- 0.003207   0.468618 +/- 0.006004   0.2351 
H4-OneShiftOnly  series 2  -2.016938 +/- 0.001926   0.427168 +/- 0.003485   0.2118 
H4-OneShiftOnly  series 3  -2.019568 +/- 0.002191   0.430474 +/- 0.004287   0.2132 
H4-OneShiftOnly  series 4  -2.014822 +/- 0.002322   0.434599 +/- 0.009917   0.2157 
H4-OneShiftOnly  series 5  -2.060509 +/- 0.001297   0.202193 +/- 0.003284   0.0981 
H4-OneShiftOnly  series 6  -2.066163 +/- 0.001567   0.205154 +/- 0.002769   0.0993 
H4-OneShiftOnly  series 7  -2.066169 +/- 0.001489   0.207286 +/- 0.003208   0.1003 
H4-OneShiftOnly  series 8  -2.065890 +/- 0.001396   0.205738 +/- 0.002847   0.0996 
H4-OneShiftOnly  series 9  -2.105417 +/- 0.001126   0.173066 +/- 0.001457   0.0822 
H4-OneShiftOnly  series 10  -2.135769 +/- 0.000914   0.152791 +/- 0.000722   0.0715 
H4-OneShiftOnly  series 11  -2.138689 +/- 0.000870   0.152215 +/- 0.000648   0.0712 
H4-OneShiftOnly  series 12  -2.141079 +/- 0.000794   0.155026 +/- 0.002604   0.0724 
H4-OneShiftOnly  series 13  -2.155559 +/- 0.001001   0.127789 +/- 0.001123   0.0593 
H4-OneShiftOnly  series 14  -2.157919 +/- 0.001123   0.130314 +/- 0.000537   0.0604 
H4-OneShiftOnly  series 15  -2.159332 +/- 0.000800   0.130835 +/- 0.001147   0.0606 

@jtkrogel
Copy link
Contributor

@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.

@prckent
Copy link
Contributor

prckent commented Aug 12, 2022

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

ye-luo commented Aug 12, 2022

@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.

@ye-luo
Copy link
Contributor Author

ye-luo commented Aug 12, 2022

I found a way to first load multiple value as a string and then convert the string to a vector of something.

@ye-luo ye-luo marked this pull request as ready for review August 17, 2022 21:20
@ye-luo ye-luo requested a review from prckent August 17, 2022 21:30
prckent
prckent previously approved these changes Aug 19, 2022
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.

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.)

@ye-luo
Copy link
Contributor Author

ye-luo commented Aug 19, 2022

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.

@prckent
Copy link
Contributor

prckent commented Aug 19, 2022

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.

@ye-luo
Copy link
Contributor Author

ye-luo commented Aug 19, 2022

Test this please

@ye-luo ye-luo enabled auto-merge August 19, 2022 16:21
@ye-luo ye-luo merged commit 13d81e2 into QMCPACK:develop Aug 19, 2022
@ye-luo ye-luo deleted the support-continue-opt branch August 19, 2022 18:28
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.

4 participants