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

FreeOrbital SPOSet #4084

Merged
merged 35 commits into from
Jul 5, 2022
Merged

FreeOrbital SPOSet #4084

merged 35 commits into from
Jul 5, 2022

Conversation

Paul-St-Young
Copy link
Contributor

@Paul-St-Young Paul-St-Young commented Jun 28, 2022

Proposed changes

This PR consolidates the ElectronGas folder into a single SPOSet object "FreeParticle".
This merges real and complex code paths into one file.

While mostly a refactor, this PR has the side effect of enabling simulation of the 2D electron gas, because it reuses KContainer, where ndim was recently introduced #3852.

I updated all the unit tests. One test needed to be dropped: "det_heg.rpa.rs5.xml" in the real build.
The dropped test uses twisted boundary condition, which the real code was never meant to support.

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

  • Refactoring (no functional changes, no api changes)

Does this introduce a breaking change?

  • Yes

Electron gas with twisted boundary condition can no longer be executed by the real code.
One must compile the complex code to use a twist.

What systems has this change been tested on?

workstation

Checklist

  • Yes. This PR is up to date with current the current state of 'develop'
  • No. Code added or changed in the PR has been clang-formatted
  • Yes. 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)

@ye-luo
Copy link
Contributor

ye-luo commented Jun 28, 2022

Test this please

@ye-luo
Copy link
Contributor

ye-luo commented Jun 28, 2022

Replace ElectronGas with something without ifdef is long wanted.

@prckent
Copy link
Contributor

prckent commented Jun 29, 2022

Thanks.

An easy to address point:

Why not retain the naming of ElectronGas or similar? Do you have a more general non-electron gas usage despite all the electron gas plumbing and formulae?

Repurposing "Particle" here is not ideal because we already have things like ParticleSets which refer to very different things. We are trying to remove these sources of confusion from the codebase.

@prckent
Copy link
Contributor

prckent commented Jun 29, 2022

I would like to see deterministic tests added for this code, with very good coverage obtained. This should be easy to achieve given the existing stochastic tests. (On my phone, so apologies if I missed them)

@Paul-St-Young
Copy link
Contributor Author

Paul-St-Young commented Jun 29, 2022

Why not retain the naming of ElectronGas or similar?

Because the single-particle orbitals for the electron gas can be free, localized (Wigner crystal), or something else.
Do you prefer something like ElectronGasFree, ElectronGasWigner etc?

I would like to see deterministic tests added for this code

Can you give more detail on the kind of tests you are looking for?
Do you mean different particle numbers, different cell shapes, or something else?

@Paul-St-Young
Copy link
Contributor Author

Now I remember: the free orbitals can be used in a simulation with boson or boltzmann statistics.
That's why I did not want to add "ElectronGas" in front of "Free".

@prckent
Copy link
Contributor

prckent commented Jun 29, 2022

Paul: the deterministic tests I was referring to use a fixed random seed and a very short run to enable fully deterministic and reproducibile results. Typically they are just a few moves, just enough to hit different code paths. These tests don't demonstrate correctness but do demonstrate consistency. We run them in the CI and recommend users run all the deterministic tests when installing qmcpack (ctest -L deterministic). They are the only way we have found to reliably keep features alive, e.g. ensuring that someone doesn't accidentally break this code . If we have a deterministic test then it will fail in CI and the attempt to break this code in future will fail. We also run sanitizers with the deterministic tests to catch memory leaks etc.

The standard way to make these is to adopt a short test. See e.g. https://github.com/QMCPACK/qmcpack/blob/develop/tests/solids/diamondC_1x1x1_pp/det_qmc_short.in.xml and https://github.com/QMCPACK/qmcpack/blob/develop/tests/solids/diamondC_1x1x1_pp/CMakeLists.txt .The former has

<?xml version="1.0"?>
<simulation>
   <project id="det_qmc_short" series="0">
      <application name="qmcapp" role="molecu" class="serial" version="1.0"/>
   </project>
   <random seed="71"/>

   <qmc method="vmc" move="pbyp">
      <estimator name="LocalEnergy" hdf5="no"/>
      <parameter name="walkers"             >    1               </parameter>
      <parameter name="blocks"              >    3               </parameter>
      <parameter name="steps"               >    3               </parameter>
      <parameter name="subSteps"            >    2               </parameter>
      <parameter name="timestep"            >    0.3             </parameter>
      <parameter name="warmupSteps"         >    3               </parameter>
   </qmc>

@prckent
Copy link
Contributor

prckent commented Jun 29, 2022

Some good news is that I was getting ahead of the actual situation - we do have some deterministic tests for the heg, the question is whether all the code paths are covered or not with these changes.
Screen Shot 2022-06-29 at 11 41 05 AM

@ye-luo
Copy link
Contributor

ye-luo commented Jun 29, 2022

How about FreeParticleOrbitals?

@prckent
Copy link
Contributor

prckent commented Jun 29, 2022

Good news, coverage is actually still very decent :-) https://app.codecov.io/gh/QMCPACK/qmcpack/compare/4084/tree/src/QMCWaveFunctions/ElectronGas (You might need to authenticate with your GitHub ID). For simplicity I suggest we put this item on the list of things to verify after this PR is in. The goal would be to craft an extra test or remove some unused legacy code to get the coverage >>80%. If there are vestigial functions from some old usage these should definitely go.

Copy link
Contributor

@ye-luo ye-luo left a comment

Choose a reason for hiding this comment

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

Seems only a few requested change got missed. Very close to merge.

void updateKLists(const ParticleLayout& lattice,
RealType kc,
unsigned ndim,
const PosType& twist = PosType(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Still missing documentation for twist. Say something about how twist affects KLists.

@@ -81,7 +81,7 @@ class KContainer : public QMCTraits
*/
void findApproxMMax(const ParticleLayout& lattice, unsigned ndim);
/** construct the container for k-vectors */
void BuildKLists(const ParticleLayout& lattice, bool useSphere);
void BuildKLists(const ParticleLayout& lattice, PosType twist, bool useSphere);
Copy link
Contributor

Choose a reason for hiding this comment

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

@Paul-St-Young did you miss this one?

@@ -71,6 +70,9 @@ class StructFact : public QMCTraits
/// accessor of StorePerParticle
bool isStorePerParticle() const { return StorePerParticle; }

/// accessor of k_lists_
const KContainer getKLists() const { return k_lists_; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Still incorrect. "const KContainer&"

@ye-luo
Copy link
Contributor

ye-luo commented Jul 5, 2022

Test this please

for (int ldim=0;ldim<ndim;ldim++)
{
CHECK(klists.kpts[ik][ldim] == gvecs[ik][ldim]);
CHECK(klists.kpts[ik][ldim]*blat == klists.kpts_cart[ik][ldim]);
Copy link
Contributor

Choose a reason for hiding this comment

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

CI failure are from missing Approx. Please double check all the CHECK. Also this file seems needing clang-format.

((ggg[j2])[la])(lb, la) = -coskr * (kvecs[ik])[la] * (kvecs[ik])[lb] * (kvecs[ik])[la];
((ggg[j1])[la])(la, lb) = ((ggg[j1])[la])(lb, la);
((ggg[j2])[la])(la, lb) = ((ggg[j2])[la])(lb, la);
((ggg[j1])[lb])(la, la) = ((ggg[j1])[la])(lb, la);
Copy link
Contributor

Choose a reason for hiding this comment

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

My eyes hurt with excessive () around hess and ggg

@ye-luo
Copy link
Contributor

ye-luo commented Jul 5, 2022

Test this please

@Paul-St-Young
Copy link
Contributor Author

@ye-luo sorry I missed some of your requests. They should all be addressed now.

ye-luo
ye-luo previously approved these changes Jul 5, 2022
@Paul-St-Young
Copy link
Contributor Author

@PDoakORNL unit tests for the KContainer have been added.
Please see if this is satisfactory now.

@ye-luo
Copy link
Contributor

ye-luo commented Jul 5, 2022

Test this please

prckent
prckent previously approved these changes Jul 5, 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.

(deleted comments)

@prckent prckent dismissed their stale review July 5, 2022 14:34

Wrong PR

@prckent prckent self-requested a review July 5, 2022 14:41
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.

This now LGTM assuming that all the tests pass. @ye-luo should still sign off. Thanks for all the updates.

@prckent
Copy link
Contributor

prckent commented Jul 5, 2022

Test this please

@prckent prckent enabled auto-merge July 5, 2022 18:12
@prckent prckent merged commit 59021cf into QMCPACK:develop Jul 5, 2022
@Paul-St-Young Paul-St-Young deleted the fporb branch July 5, 2022 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants