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.

Sign up for GitHub

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

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

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
40df6ec
don't allow batched version to except hamiltonian estimator input
PDoakORNL Jun 22, 2023
1c25758
add test case
PDoakORNL Jun 22, 2023
0016e57
fixed a number of problems with the legacy Estimator input blocking
PDoakORNL Jun 23, 2023
4590f20
fix many tests that still use flux scalar estimator
PDoakORNL Jun 23, 2023
72890e9
work around bad initialization analysis in GCC11
PDoakORNL Jun 30, 2023
2d1bf88
address @ye-luo's review
PDoakORNL Jul 5, 2023
3101da4
fix typo
PDoakORNL Jul 5, 2023
bd73bda
Merge branch 'develop' into protect_from_estimator_input_in_qmchamilt…
prckent Jul 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/QMCApp/QMCMain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,8 @@ bool QMCMain::validateXML()
}
else if (cname == "hamiltonian")
{
ham_pool_->put(cur);
// second param enables batched input error checking in HamiltonianFactory
ham_pool_->put(cur, my_project_.getDriverVersion() == ProjectData::DriverVersion::BATCH);
}
else if (cname == "include")
{
Expand Down Expand Up @@ -546,7 +547,7 @@ bool QMCMain::processPWH(xmlNodePtr cur)
else if (cname == "hamiltonian")
{
inputnode = true;
ham_pool_->put(cur);
ham_pool_->put(cur, my_project_.getDriverVersion() == ProjectData::DriverVersion::BATCH);
}
else if (cname == "estimators")
{
Expand Down
19 changes: 15 additions & 4 deletions src/QMCHamiltonians/HamiltonianFactory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// This file is distributed under the University of Illinois/NCSA Open Source License.
// See LICENSE file in top directory for details.
//
// Copyright (c) 2016 Jeongnim Kim and QMCPACK developers.
// Copyright (c) 2023 QMCPACK developers.
//
// File developed by: John R. Gergely, University of Illinois at Urbana-Champaign
// Bryan Clark, [email protected], Princeton University
Expand Down Expand Up @@ -49,6 +49,7 @@
#endif
#include "QMCHamiltonians/SkPot.h"
#include "OhmmsData/AttributeSet.h"
#include "Message/UniformCommunicateError.h"

namespace qmcplusplus
{
Expand Down Expand Up @@ -84,7 +85,7 @@ HamiltonianFactory::HamiltonianFactory(const std::string& hName,
* </hamiltonian>
* \endxmlonly
*/
bool HamiltonianFactory::build(xmlNodePtr cur)
bool HamiltonianFactory::build(xmlNodePtr cur, bool batched)
{
if (cur == NULL)
return false;
Expand Down Expand Up @@ -176,8 +177,18 @@ bool HamiltonianFactory::build(xmlNodePtr cur)
targetH->addOperator(std::move(hs), "Grid", true);
}
}
else if (cname == "flux")
{
if (potName.size() < 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (potName.size() < 1)
if (potName.empty())

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

potName = "flux";
targetH->addOperator(std::make_unique<ConservedEnergy>(), potName, false);
}
else if (cname == "estimator")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@PDoakORNL PDoakORNL Jul 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

{
if (batched)
throw UniformCommunicateError(
"estimator input nodes are not accepted in hamiltonian input in the batched version of the code!");

if (potType == "flux")
targetH->addOperator(std::make_unique<ConservedEnergy>(), potName, false);
else if (potType == "specieskinetic")
Expand Down Expand Up @@ -418,9 +429,9 @@ void HamiltonianFactory::renameProperty(std::string& aname)
}
}

bool HamiltonianFactory::put(xmlNodePtr cur)
bool HamiltonianFactory::put(xmlNodePtr cur, bool batched)
{
bool success = build(cur);
bool success = build(cur, batched);
return success;
}

Expand Down
9 changes: 6 additions & 3 deletions src/QMCHamiltonians/HamiltonianFactory.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ class HamiltonianFactory : public MPIObjectBase
Communicate* c);

///read from xmlNode
bool put(xmlNodePtr cur);
bool put(xmlNodePtr cur, bool batched);

/** add a property whose name will be renamed by b
* @param a target property whose name should be replaced by b
Expand All @@ -58,9 +58,12 @@ class HamiltonianFactory : public MPIObjectBase
QMCHamiltonian* getH() const { return targetH.get(); }

private:
/** process xmlNode to populate targetPsi
/** process xmlNode to populate the targetH
* \param[in] cur xml that contains the actual arguments required to make a valid hamiltonians in the QMCHamiltonian
* \param[in] batched if true reject legacy input for estimators.
* returns true unless cur == NULL
*/
bool build(xmlNodePtr cur);
bool build(xmlNodePtr cur, bool batched);

void addCoulombPotential(xmlNodePtr cur);
void addForceHam(xmlNodePtr cur);
Expand Down
7 changes: 4 additions & 3 deletions src/QMCHamiltonians/HamiltonianPool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ HamiltonianPool::~HamiltonianPool()
}
}

bool HamiltonianPool::put(xmlNodePtr cur)
bool HamiltonianPool::put(xmlNodePtr cur, bool batched)
{
ReportEngine PRE("HamiltonianPool", "put");
std::string id("h0"), target("e"), role("extra");
Expand Down Expand Up @@ -77,15 +77,16 @@ bool HamiltonianPool::put(xmlNodePtr cur)
}
else
curH = (*hit).second;
bool success = curH->put(cur);

bool success = curH->put(cur, batched);
if (set2Primary)
primaryH = curH->getH();
return success;
}

bool HamiltonianPool::get(std::ostream& os) const
{
for(auto& [name, factory] : myPool)
for (auto& [name, factory] : myPool)
{
os << " Hamiltonian " << name << std::endl;
factory->getH()->get(os);
Expand Down
7 changes: 6 additions & 1 deletion src/QMCHamiltonians/HamiltonianPool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,12 @@ class HamiltonianPool : public MPIObjectBase
HamiltonianPool& operator=(HamiltonianPool&&) = delete;
~HamiltonianPool();

bool put(xmlNodePtr cur);
/** put method that actually constructs the object.
* \param[in] cur xml that contains the actual arguments required to make a valid hamiltonians in the QMCHamiltonian
* \param[in] batched if true reject legacy input for estimators.
* returns if put was successful, ignored in QMCMain.cpp!
*/
bool put(xmlNodePtr cur, bool batched);
bool get(std::ostream& os) const;
void reset();

Expand Down
6 changes: 3 additions & 3 deletions src/QMCHamiltonians/tests/MinimalHamiltonianPool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// This file is distributed under the University of Illinois/NCSA Open Source License.
// See LICENSE file in top directory for details.
//
// Copyright (c) 2019 QMCPACK developers.
// Copyright (c) 2023 QMCPACK developers.
//
// File developed by: Peter Doak, [email protected], Oak Ridge National Laboratory
//
Expand Down Expand Up @@ -45,7 +45,7 @@ class MinimalHamiltonianPool
doc.parseFromString(hamiltonian_xml);

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

return hpool;
}
Expand All @@ -59,7 +59,7 @@ class MinimalHamiltonianPool
doc.parseFromString(hamiltonian_eeei_xml);

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

return hpool;
}
Expand Down
2 changes: 1 addition & 1 deletion src/QMCHamiltonians/tests/test_RotatedSPOs_NLPP.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ void test_hcpBe_rotation(bool use_single_det, bool use_nlpp_batched)
REQUIRE(okay2);

xmlNodePtr root2 = doc2.getRoot();
hf.put(root2);
hf.put(root2,true);

opt_variables_type opt_vars;
psi->checkInVariables(opt_vars);
Expand Down
44 changes: 42 additions & 2 deletions src/QMCHamiltonians/tests/test_hamiltonian_factory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "QMCWaveFunctions/WaveFunctionFactory.h"
#include "QMCHamiltonians/HamiltonianFactory.h"
#include "Utilities/RuntimeOptions.h"
#include "Message/UniformCommunicateError.h"

namespace qmcplusplus
{
Expand Down Expand Up @@ -75,7 +76,7 @@ TEST_CASE("HamiltonianFactory", "[hamiltonian]")
REQUIRE(okay);

xmlNodePtr root = doc.getRoot();
hf.put(root);
hf.put(root, true);


REQUIRE(hf.getH());
Expand All @@ -86,6 +87,45 @@ TEST_CASE("HamiltonianFactory", "[hamiltonian]")
REQUIRE(hf.getH()->getOperatorType("ElecIon") == "coulomb");
}

TEST_CASE("HamiltonianFactory_prevent_legacy_estimator_input", "[hamiltonian]")
{
Communicate* c = OHMMS::Controller;

const SimulationCell simulation_cell;
auto elec_ptr = createElectronParticleSet(simulation_cell);
auto ions_ptr = std::make_unique<ParticleSet>(simulation_cell);

auto &ions(*ions_ptr), elec(*elec_ptr);

ions.setName("ion0");
ions.create({1});

HamiltonianFactory::PSetMap particle_set_map;
particle_set_map.emplace(ions_ptr->getName(), std::move(ions_ptr));
particle_set_map.emplace(elec_ptr->getName(), std::move(elec_ptr));

RuntimeOptions runtime_options;
HamiltonianFactory::PsiPoolType psi_map;
psi_map.emplace("psi0", WaveFunctionFactory::buildEmptyTWFForTesting(runtime_options, "psi0"));

HamiltonianFactory hf("h0", elec, particle_set_map, psi_map, c);

const char* hamiltonian_xml = R"(<hamiltonian name="h0" type="generic" target="e">
<pairpot type="coulomb" name="ElecElec" source="e" target="e"/>
<pairpot type="coulomb" name="IonIon" source="ion0" target="ion0"/>
<pairpot type="coulomb" name="ElecIon" source="ion0" target="e"/>
<estimator name="Density" type="density" delta="0.1 0.1 0.1"/>
</hamiltonian>)";

Libxml2Document doc;
bool okay = doc.parseFromString(hamiltonian_xml);
REQUIRE(okay);

xmlNodePtr root = doc.getRoot();

CHECK_THROWS_AS(hf.put(root, true), qmcplusplus::UniformCommunicateError);
}

TEST_CASE("HamiltonianFactory pseudopotential", "[hamiltonian]")
{
Communicate* c = OHMMS::Controller;
Expand Down Expand Up @@ -128,7 +168,7 @@ TEST_CASE("HamiltonianFactory pseudopotential", "[hamiltonian]")
REQUIRE(okay);

xmlNodePtr root = doc.getRoot();
hf.put(root);
hf.put(root,true);
}

} // namespace qmcplusplus
2 changes: 1 addition & 1 deletion src/QMCHamiltonians/tests/test_hamiltonian_pool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ TEST_CASE("HamiltonianPool", "[qmcapp]")

REQUIRE(hpool.empty());

hpool.put(root);
hpool.put(root,true);

QMCHamiltonian* h = hpool.getHamiltonian("h0");
REQUIRE(h != nullptr);
Expand Down
2 changes: 1 addition & 1 deletion src/QMCHamiltonians/tests/test_ion_derivs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ QMCHamiltonian& create_CN_Hamiltonian(HamiltonianFactory& hf)
REQUIRE(okay);

xmlNodePtr root = doc.getRoot();
hf.put(root);
hf.put(root,true);

return *hf.getH();
}
Expand Down
2 changes: 1 addition & 1 deletion tests/io/restart_batch/qmc_short_batch.in.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,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 type="flux" name="Flux"/>
</hamiltonian>
</qmcsystem>
<qmc method="vmc_batch" move="pbyp" checkpoint="0">
Expand Down
2 changes: 1 addition & 1 deletion tests/io/restart_batch/qmc_short_batch.restart.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,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 type="flux" name="Flux"/>
</hamiltonian>
</qmcsystem>

Expand Down
1 change: 0 additions & 1 deletion tests/io/restart_dmc/det_qmc_vmcbatch_dmcbatch_tm.in.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@
<pseudo elementType="C" href="C.BFD.xml"/>
</pairpot>
<pairpot name="MPC" type="MPC" source="e" target="e" ecut="60.0" physical="false"/>
<!-- <estimator type="flux" name="Flux"/> -->
</hamiltonian>
</qmcsystem>

Expand Down
1 change: 0 additions & 1 deletion tests/io/restart_dmc/det_qmc_vmcbatch_dmcbatch_tm.restart.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@
<pseudo elementType="C" href="C.BFD.xml"/>
</pairpot>
<pairpot name="MPC" type="MPC" source="e" target="e" ecut="60.0" physical="false"/>
<!-- <estimator type="flux" name="Flux"/> -->
</hamiltonian>
</qmcsystem>

Expand Down
1 change: 0 additions & 1 deletion tests/io/restart_dmc_disable_branching/det_qmc_vmcbatch_dmcbatch_tm.in.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@
<pseudo elementType="C" href="C.BFD.xml"/>
</pairpot>
<pairpot name="MPC" type="MPC" source="e" target="e" ecut="60.0" physical="false"/>
<!-- <estimator type="flux" name="Flux"/> -->
</hamiltonian>
</qmcsystem>

Expand Down
1 change: 0 additions & 1 deletion tests/io/restart_dmc_disable_branching/det_qmc_vmcbatch_dmcbatch_tm.restart.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@
<pseudo elementType="C" href="C.BFD.xml"/>
</pairpot>
<pairpot name="MPC" type="MPC" source="e" target="e" ecut="60.0" physical="false"/>
<!-- <estimator type="flux" name="Flux"/> -->
</hamiltonian>
</qmcsystem>

Expand Down
2 changes: 1 addition & 1 deletion tests/solids/diamondC_1x1x1_pp/det_qmc_param_grad.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@
<pseudo elementType="C" href="C.BFD.xml"/>
</pairpot>
<pairpot name="MPC" type="MPC" source="e" target="e" ecut="60.0" physical="false"/>
<estimator type="flux" name="Flux"/>
<flux type="flux" name="Flux"/>
</hamiltonian>
</qmcsystem>
<loop max="1">
Expand Down
2 changes: 1 addition & 1 deletion tests/solids/diamondC_1x1x1_pp/det_qmc_spindens_short_vmcbatched.in.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,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 type="flux" name="Flux"/>
</hamiltonian>
</qmcsystem>
<qmc method="vmc" move="pbyp">
Expand Down
1 change: 0 additions & 1 deletion tests/solids/diamondC_1x1x1_pp/det_qmc_vmcbatch_dmcbatch_tm.in.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@
<pseudo elementType="C" href="C.BFD.xml"/>
</pairpot>
<pairpot name="MPC" type="MPC" source="e" target="e" ecut="60.0" physical="false"/>
<!-- <estimator type="flux" name="Flux"/> -->
</hamiltonian>
</qmcsystem>

Expand Down
2 changes: 1 addition & 1 deletion tests/solids/diamondC_1x1x1_pp/qmc_short_optbatch.in.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,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"/>
</hamiltonian>
</qmcsystem>

Expand Down
2 changes: 1 addition & 1 deletion tests/solids/diamondC_1x1x1_pp/qmc_spindens_vmcbatch_short.in.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,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 type="flux" name="Flux"/>
</hamiltonian>
<estimators>
<estimator name="spindensity" type="spindensity" report="yes" save_memory="no">
Expand Down
2 changes: 1 addition & 1 deletion tests/solids/diamondC_2x1x1_pp/det_qmc_short_sdbatch_vmcbatch_dmcbatch_mwalkers.in.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,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 type="flux" name="Flux"/>
</hamiltonian>
</qmcsystem>

Expand Down
2 changes: 1 addition & 1 deletion ...s/diamondC_2x1x1_pp/det_qmc_short_sdbatch_vmcbatch_dmcbatch_mwalkers_inverter_host.in.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,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"/>
</hamiltonian>
</qmcsystem>

Expand Down
2 changes: 1 addition & 1 deletion ...ids/diamondC_2x1x1_pp/det_qmc_short_sdbatch_vmcbatch_dmcbatch_mwalkers_with_checks.in.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,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"/>
</hamiltonian>
</qmcsystem>

Expand Down
2 changes: 1 addition & 1 deletion tests/solids/diamondC_2x1x1_pp/det_qmc_short_sdbatch_vmcbatch_mwalkers.in.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,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"/>
</hamiltonian>
</qmcsystem>

Expand Down
Loading