Don't allow batched version to accept hamiltonian estimator input #4643

PDoakORNL · 2023-06-22T23:31:32Z

Proposed changes

Prevent legacy estimator inputs in batched input.
This addresses issue #4637

What type(s) of changes does this code introduce?

Delete the items that do not apply

New feature

Does this introduce a breaking change?

Yes if a user has these previous ignored input elements in the hamiltonian an exception is thrown and the code halts.

What systems has this change been tested on?

osx laptop

Checklist

Update the following with a yes where the items apply. If you're unsure about any of them, don't hesitate to ask. This is
simply a reminder of what we are going to look for before merging your code.

Yes This PR is up to date with current the current state of 'develop'
Yes Code added or changed in the PR has been clang-formatted
No. This PR adds tests to cover any new code, or to catch a bug that is being fixed
Yes Documentation has been added (if appropriate)

quantumsteve · 2023-07-01T01:09:16Z

src/QMCHamiltonians/HamiltonianFactory.cpp

@@ -176,8 +177,18 @@ bool HamiltonianFactory::build(xmlNodePtr cur)
        targetH->addOperator(std::move(hs), "Grid", true);
      }
    }
+    else if (cname == "flux")
+    {
+      if (potName.size() < 1)


Suggested change

if (potName.size() < 1)

if (potName.empty())

I'm thinking of adding legacy_estimator? there is no guarantee we make the legacy ones working but at least users may have a chance to force it.

I'm not in favor of that as it will inhibit refactoring QMCHamiltonian. If there is a batched version the legacy version should be disabled for a batched run.

I think by the finish of ecp the manual should not include mention of using the tag estimator in the Hamiltonian input node.

I'm not in favor of that as it will inhibit refactoring QMCHamiltonian. If there is a batched version the legacy version should be disabled for a batched run.

If there is a batched version, the old input through Hamiltonian should be blocked. What if not? I would prefer giving user a chance to try out things without modifying the source code.

I think by the finish of ecp the manual should not include mention of using the tag estimator in the Hamiltonian input node.

I agree with this but the reality is we don't port estimators fast enough.

ye-luo · 2023-07-03T05:03:48Z

src/QMCApp/QMCMain.cpp

+        batched = true;
+        break;
+      }
+      ham_pool_->put(cur, batched);


Could the following work?

ham_pool_->put(cur, my_project_.getDriverVersion() == ProjectData::DriverVersion::BATCH);

If it works, please change the following place as well.

ye-luo · 2023-07-03T05:10:28Z

src/QMCHamiltonians/tests/MinimalHamiltonianPool.h

@@ -45,7 +45,7 @@ class MinimalHamiltonianPool
    doc.parseFromString(hamiltonian_xml);

    xmlNodePtr root = doc.getRoot();
-    hpool.put(root);
+    hpool.put(root,true);


Please apply clang-format. There should be a space after the comma.

ye-luo · 2023-07-03T05:13:55Z

src/QMCHamiltonians/HamiltonianFactory.cpp

@@ -176,8 +177,18 @@ bool HamiltonianFactory::build(xmlNodePtr cur)
        targetH->addOperator(std::move(hs), "Grid", true);
      }
    }
+    else if (cname == "flux")
+    {
+      if (potName.size() < 1)


I'm thinking of adding legacy_estimator? there is no guarantee we make the legacy ones working but at least users may have a chance to force it.

ye-luo · 2023-07-03T05:17:00Z

src/QMCHamiltonians/HamiltonianFactory.cpp

+      if (potName.size() < 1)
+	potName = "flux";
+      targetH->addOperator(std::make_unique<ConservedEnergy>(), potName, false);
+    }
    else if (cname == "estimator")


Change here like cname == "estimator" || cname == "legacy_estimator" and the error check as
if (batched && cname == "estimator")

I disagree with this change, legacy estimators that are going to continue to be allowed should be explicitly changed as the flux estimator has been. They are not "estimators" by the definition we will be using coming forward. They should be renabled individually if necessary. Further discussion of this should return to #4637 which this PR addresses, if this is the functionality you think is correct argue for it in #4637 with @jtkrogel and we can amend this PR or make another one.

Changing to flux or legacy_estimator, I prefer the latter a more generic way.
We can only allow using legacy_estimator when batched driver is in use and the selected estimator doesn't have an actual batched implementation.

ye-luo · 2023-07-03T05:17:52Z

tests/solids/diamondC_2x1x1_pp/qmc_short_vmcbatch_dmcbatch.in.xml

@@ -80,7 +80,7 @@
         <pairpot type="pseudo" name="PseudoPot" source="ion0" wavefunction="psi0" format="xml">
            <pseudo elementType="C" href="C.BFD.xml"/>
         </pairpot>
-         <!-- <estimator type="flux" name="Flux"/> -->
+         <!-- <flux name="Flux"/> -->


Delete this line.

ye-luo · 2023-07-03T05:17:57Z

tests/solids/diamondC_2x1x1_pp/qmc_short_vmcbatch_dmcbatch_4r.in.xml

@@ -80,7 +80,7 @@
         <pairpot type="pseudo" name="PseudoPot" source="ion0" wavefunction="psi0" format="xml">
            <pseudo elementType="C" href="C.BFD.xml"/>
         </pairpot>
-         <!-- <estimator type="flux" name="Flux"/> -->
+         <!-- <flux name="Flux"/> -->


Delete this line.

PDoakORNL

Addressed review

PDoakORNL · 2023-07-05T17:50:32Z

src/QMCHamiltonians/HamiltonianFactory.cpp

@@ -176,8 +177,18 @@ bool HamiltonianFactory::build(xmlNodePtr cur)
        targetH->addOperator(std::move(hs), "Grid", true);
      }
    }
+    else if (cname == "flux")
+    {
+      if (potName.size() < 1)


I'm not in favor of that as it will inhibit refactoring QMCHamiltonian. If there is a batched version the legacy version should be disabled for a batched run.

PDoakORNL · 2023-07-05T18:23:11Z

src/QMCHamiltonians/tests/MinimalHamiltonianPool.h

@@ -45,7 +45,7 @@ class MinimalHamiltonianPool
    doc.parseFromString(hamiltonian_xml);

    xmlNodePtr root = doc.getRoot();
-    hpool.put(root);
+    hpool.put(root,true);


PDoakORNL · 2023-07-05T18:24:19Z

src/QMCHamiltonians/HamiltonianFactory.cpp

@@ -176,8 +177,18 @@ bool HamiltonianFactory::build(xmlNodePtr cur)
        targetH->addOperator(std::move(hs), "Grid", true);
      }
    }
+    else if (cname == "flux")
+    {
+      if (potName.size() < 1)


I think by the finish of ecp the manual should not include mention of using the tag estimator in the Hamiltonian input node.

PDoakORNL · 2023-07-05T19:46:01Z

src/QMCApp/QMCMain.cpp

+        batched = true;
+        break;
+      }
+      ham_pool_->put(cur, batched);


PDoakORNL

I consider this PR complete in its solution to #4637. I will not further update it to track develop unless it is approved.

PDoakORNL · 2023-07-17T19:25:14Z

src/QMCHamiltonians/HamiltonianFactory.cpp

+      if (potName.size() < 1)
+	potName = "flux";
+      targetH->addOperator(std::make_unique<ConservedEnergy>(), potName, false);
+    }
    else if (cname == "estimator")


I disagree with this change, legacy estimators that are going to continue to be allowed should be explicitly changed as the flux estimator has been. They are not "estimators" by the definition we will be using coming forward. They should be renabled individually if necessary. Further discussion of this should return to #4637 which this PR addresses, if this is the functionality you think is correct argue for it in #4637 with @jtkrogel and we can amend this PR or make another one.

prckent · 2023-07-21T13:55:51Z

Taking a look at this, we are not going to add legacy_estimator in future. We need to focus on estimators for the batched code and not get entangled with supporting more legacy. This would take time, resources and tackling complexity we should simply not engage with. Users should use updated estimators for the batched code or simply run an older version until their needed estimator is available.

@jtkrogel Are you OK with <flux name="Flux"/> and the changes here? My opinion is that this is mergable now.

…onian_in_batched

prckent · 2023-07-21T14:00:48Z

Test this please

jtkrogel · 2023-07-21T14:14:14Z

I don't understand the need for changing the notation around "flux" (<flux/>). It is a statistical estimator (<estimator/>). It should be ported (or not) to the batched code under the same conditions as all the rest.

For the purpose of this PR, the only requirement I see is that <estimator/>'s are banished from <hamiltonian/>. The priority around moving other estimators to the batched code framework is separate.

Perhaps we just learned that "flux" is a higher priority estimator for porting than others. I don't think that should hold up the rest of the work here.

ye-luo · 2023-07-21T14:27:34Z

I feel unnecessary to introduce <flux name="Flux"/>. If we decide to allow such estimators still in the Hamiltonian input section, having legacy_estimator input style is more generic. Users needs to opt-in if they really want to try such a feature and the experience is allowed to be a hit and miss. In the current situation, we block everything and porting is slow.

PDoakORNL · 2023-07-25T13:35:56Z

I think it is desirable to block everything that has not been examined and properly ported.

prckent changed the title ~~Don't allow batched version to except hamiltonian estimator input~~ Don't allow batched version to accept hamiltonian estimator input Jun 24, 2023

PDoakORNL marked this pull request as ready for review June 30, 2023 21:08

quantumsteve reviewed Jul 1, 2023

View reviewed changes

ye-luo reviewed Jul 3, 2023

View reviewed changes

PDoakORNL commented Jul 5, 2023

View reviewed changes

PDoakORNL commented Jul 17, 2023

View reviewed changes

PDoakORNL added 7 commits July 17, 2023 15:29

don't allow batched version to except hamiltonian estimator input

40df6ec

add test case

1c25758

fixed a number of problems with the legacy Estimator input blocking

0016e57

fix many tests that still use flux scalar estimator

4590f20

work around bad initialization analysis in GCC11

72890e9

address @ye-luo's review

2d1bf88

fix typo

3101da4

PDoakORNL force-pushed the protect_from_estimator_input_in_qmchamiltonian_in_batched branch from b4b4c40 to 3101da4 Compare July 17, 2023 19:29

PDoakORNL enabled auto-merge (squash) July 17, 2023 19:29

Merge branch 'develop' into protect_from_estimator_input_in_qmchamilt…

bd73bda

…onian_in_batched

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't allow batched version to accept hamiltonian estimator input #4643

Don't allow batched version to accept hamiltonian estimator input #4643

PDoakORNL commented Jun 22, 2023

quantumsteve Jul 1, 2023

ye-luo Jul 3, 2023

PDoakORNL Jul 5, 2023

PDoakORNL Jul 5, 2023

ye-luo Jul 21, 2023

ye-luo Jul 21, 2023

ye-luo Jul 3, 2023

PDoakORNL Jul 5, 2023

ye-luo Jul 3, 2023

PDoakORNL Jul 5, 2023

ye-luo Jul 3, 2023

ye-luo Jul 3, 2023

PDoakORNL Jul 17, 2023 •

edited

Loading

ye-luo Jul 21, 2023

ye-luo Jul 3, 2023

ye-luo Jul 3, 2023

PDoakORNL left a comment

PDoakORNL Jul 5, 2023

PDoakORNL Jul 5, 2023

PDoakORNL Jul 5, 2023

PDoakORNL Jul 5, 2023

PDoakORNL left a comment •

edited

Loading

PDoakORNL Jul 17, 2023 •

edited

Loading

prckent commented Jul 21, 2023 •

edited

Loading

prckent commented Jul 21, 2023

jtkrogel commented Jul 21, 2023

ye-luo commented Jul 21, 2023

PDoakORNL commented Jul 25, 2023

Don't allow batched version to accept hamiltonian estimator input #4643

Are you sure you want to change the base?

Don't allow batched version to accept hamiltonian estimator input #4643

Conversation

PDoakORNL commented Jun 22, 2023

Proposed changes

What type(s) of changes does this code introduce?

Does this introduce a breaking change?

What systems has this change been tested on?

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PDoakORNL Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PDoakORNL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PDoakORNL left a comment • edited Loading

Choose a reason for hiding this comment

PDoakORNL Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

prckent commented Jul 21, 2023 • edited Loading

prckent commented Jul 21, 2023

jtkrogel commented Jul 21, 2023

ye-luo commented Jul 21, 2023

PDoakORNL commented Jul 25, 2023

PDoakORNL Jul 17, 2023 •

edited

Loading

PDoakORNL left a comment •

edited

Loading

PDoakORNL Jul 17, 2023 •

edited

Loading

prckent commented Jul 21, 2023 •

edited

Loading