qmcpack icon indicating copy to clipboard operation
qmcpack copied to clipboard

Batched drivers no longer take "checkpoint" as an XML attribute

Open jtkrogel opened this issue 4 years ago • 6 comments

Issue requested by @ye-luo. While designing road tests of the new drivers, I noticed some of the XML inputs have moved around and changed form.

The purpose of this issue is to decide what format is desired.

The legacy input uses only a combination of XML attributes and parameters to represent simple values:

   <qmc method="vmc" move="pbyp" checkpoint="50">
      <parameter name="walkers"             >    1               </parameter>
      <parameter name="warmupSteps"         >    50              </parameter>
      <parameter name="blocks"              >    800             </parameter>
      <parameter name="steps"               >    10              </parameter>
      <parameter name="subSteps"            >    3               </parameter>
      <parameter name="timestep"            >    0.3             </parameter>
   </qmc>

The batched input adds new, distinct XML child elements to represent some of these simple values, in addition to attributes and parameters e.g.:

   <qmc method="vmc_batch" move="pbyp">
      <parameter name="warmupSteps"         >    50              </parameter>
      <parameter name="blocks"              >    800             </parameter>
      <parameter name="steps"               >    10              </parameter>
      <parameter name="subSteps"            >    3               </parameter>
      <parameter name="timestep"            >    0.3             </parameter>
      <checkpoint period="50" stride="..."/>
      <dumpconfig period="..." stride="..."/>
      <record period="..." stride="..."/>
      <random/>
   </qmc>

The last element (<random/>). Represents a boolean flag (true if present, false if absent).

jtkrogel avatar Feb 11 '21 16:02 jtkrogel

What do the new tags actually mean and do?

checkpoint period, stride?

dumpconfig period, stride?

random?

prckent avatar Feb 11 '21 16:02 prckent

Some points on input this brings up:

More fundamental. The parameter tag

Reads as noise to this human.

To the application its convenient because ParameterSet. Although a similar parsing class could as easily just use child tag names. It also facilitates creating runtime stringified intermediate data structures which aren't good for clarity or testing. But It would be a radical change for users to eliminate and and as long as a QMCThingInput in defined and the parameter values have not raised error on being converted into it that's all I intend for values of tags as well.

  1. Is checkpointing a "required" feature for a QMC method? If not it should defined in a child node. From a parsing standpoint Attributes in SGML tagged data formats allow particular data handling without have to unwrap child nodes. They should avoid containing attributes that aren't "universal" to the "tag type."
    On the other hand sometimes they can aid readbility seems like it should be the value or attribute of some tag like
<initial_positions>random</initial_positions>
<initial_positions from="random>

PDoakORNL avatar Feb 11 '21 18:02 PDoakORNL

Three points from my POV:

  1. Keyword-value pairs are easy to work with, and <parameter>'s model these just fine, if verbosely.

  2. Custom elements bypass the established ParameterSet and AttributeSet classes, requiring custom loop code (extra work) in QMCPACK and getting us further away from checking the validity of the input in QMCPACK in any kind of uniform way.

  3. Custom elements will require much more work to support in Nexus. While I can do it, I would prefer to have a compelling reason.

jtkrogel avatar Feb 11 '21 18:02 jtkrogel

Point 3 is pretty compelling.

PDoakORNL avatar Feb 11 '21 18:02 PDoakORNL

#4646 made the batched drivers behaving the same as legacy drivers when checkpoint is given as an attribute. I don't see a strong need to make breaking changes to the input. I would advise removing any handling of checkpoint/dumpconfig/record input as parameters.

ye-luo avatar Jun 26 '23 20:06 ye-luo

Good reminder about this issue. I think that its age indicates we have too many options in the code (legacy and batched).

I was proceeding on the basis that we would support checkpoint only, and do so using the same input as for the old drivers to avoid making a breaking change. Hence any other tags can be removed.

prckent avatar Jun 26 '23 20:06 prckent