Estimators should have containing node

[PDoakORNL]

Estimators tags should be enclosed in containing node

<qmc method="vmc_batched">
<estimators>
     <estimator name="LocalEnergy" hdf5="no"/>
     <estimator name="SpinDensityNew" type="spindensity_new" report="yes" save_memory="no">
       <parameter name="grid">
         10 10 10
       </parameter>
       <!-- this is the default in the legacy implementation -->
       <parameter name="corner">
         0.0 0.0 0.0
       </parameter>
       <parameter name="cell">
                  3.37316115        3.37316115        0.00000000
                  0.00000000        3.37316115        3.37316115
                  3.37316115        0.00000000        3.37316115
       </parameter>
     </estimator>
</estimators>
...
</qmc>

This would allow EstimatorManagerInput to be responsible for the parsing of everything within that node and allow it to raise error if it can't parse child nodes or dispatch them to specific estimator input classes.

Currently the EstimatorManagerNew gets the driver node and scans through the child nodes reading the estimator nodes as the legacy estimator manager did.

Describe the solution you'd like As above

Describe alternatives you've considered Pass collection of child nodes from the driver. Driver somehow keeps track that those nodes were delegated.

Additional context This is more consistent with the way Hamiltonians and WaveFunctions are defined. Estimator Sets could be named defined at "global" scope, and used by name in qmc sections i.e.

<estimators name="best_estimator_set">
     <estimator name="LocalEnergy" hdf5="no"/>
     <estimator name="SpinDensityNew" type="spindensity_new" report="yes" save_memory="no">
...
     </estimator>
</estimators>

<qmc method="vmc_batch">
<estimators use="best_estimator_set"/>
...
</qmc>

or
<qmc method="vmc_batch">
<estimators>best_estimator_set</estimators>
...
</qmc>

Whether this is a good idea is debatable.

[jtkrogel]

I strongly prefer we follow the patter of all other non-driver sections of the input file:

<simulation>
   <project id="qmc" series="0">
      ...
   </project>
   <qmcsystem>
      <simulationcell>
        ...
      </simulationcell>
      <particleset name="e" random="yes">
         ...
      </particleset>
      <particleset name="ion0">
         ...
      </particleset>
      <wavefunction name="psi0" target="e">
         ...
      </wavefunction>
      <hamiltonian name="h0" type="generic" target="e">
         ...
      </hamiltonian>
      <estimators>
        <estimator name="LocalEnergy" hdf5="no"/>
        <estimator name="SpinDensity" type="spindensity">
          ...
        </estimator>
     </estimators>
   </qmcsystem>
   <qmc method="vmc_batch">
      ...
   </qmc>
   <qmc method="dmc_batch">
      ...
   </qmc>
   <qmc method="dmc_batch">
      ...
   </qmc>
</simulation>

Make things simple/readable in the input file and least needed change from past styles. Instantiation of the underlying estimator objects (per driver) can be deferred until driver construction if desired, but this is a small change under the hood and not worth bubbling up to the input file.

[PDoakORNL]

So we support global estimators but the semantics must be clear. I propose Global estimators are permanent, they are present for all sections and it is illegal to define another estimator of the same name.

If you want an estimator to only be present for some sections redefine it in each section.

---

Every estimator must specify a type. Its name is a tag to refer to in output and possibly allow it to be found at runtime, but it should not control which type of estimator is in the code for some subset of estimators.

The are currently two kinds of magical name semantics. The first is in the legacy code adding another estimator with a matching name replaces the estimator that previously had that name. This seems to have been needed since the estimator manager had a indefinite lifetime. The batched estimator does not outlive its containing driver.

This also allows replacement of an estimator defined globally from the current section onward with one matching the name but type and/or definition. This creates uneeded complexity in the manager and violates the priniciple of least surprise.

The second is around estimators with no type. These refer to scalar estimators which seem to have a one to one relationship to certain drivers. Depending estimators name in input they will be one of several types of ScalarEstimators. You can actually add as many of them if you like. But only the last one will be kept and its name in the estimator map will be "LocalEnergy" regardless of its name attribute in input. This is inconsistent with how other estimators are handled. It's an ugly hack for the "MainEstimator" when different drivers expect that to be different but also makes this global estimator replacement feature.

I think they need to be changed to have no name or a name="LocalEnergy" and the type should map to a particular estimator type as will all other estimators.

<estimator type="locallnergy" hdf5=true>
or
<estimator type="rmc" nobs=20>
or
<estimator type="cslocalenergy" npsi=1>

[ye-luo]

I'd like to disallow estimator without a containing "estimators" node in batched driver input, namely

   <qmc method="dmc_batch">
      <estimator type="rmc nobs=20>
   </qmc>

is disallowed.