[WIP] Anisotropic Ewald code #2182

jtkrogel · 2020-01-03T20:55:44Z

This PR contains an adaptive anisotropic Ewald code, similar to the reference code previously developed.

The anisotropicEwald class can operate in two modes:

Perform Ewald sums for all particle pairs while building up adaptive anisotropic grids until the sum converges. The grid stops growing in directions that do not contribute. The maximum grid size in each direction is recorded across all pairs in the sum (and subsequent evaluations if desired).
Perform Ewald sums over a fixed rectangular grid that was determined during the adaptive phase. Grid sums of this type are much faster, and are at least as accurate as those determined during the adaptive phase.

This code was used to test the anisotropic nature of the Ewald sum in the quasi-2D graphene case for cells with vacuum ranging from 10-200 Bohr. For fixed accuracy, more points are needed in the z direction of the Ewald sum, while currently QMCPACK sums over a sphere (3x more points for z=40, 5x for z=200).

The resulting adaptive anisotropic grids contained 0.5 to 3 times the number of points used by QMCPACK by default in the k-space sum. In all cases tested (graphene, NiO) the adaptive code significantly outperformed QMCPACK's optimized breakup and Ewald3D implementations while requiring a similar number of points.

For this reason, I think we should consider replacing QMCPACK's LR code with an optimized version of the code in this PR.

qmc-robot · 2020-01-03T20:56:01Z

Can one of the admins verify this patch?

ye-luo

A bit more documentation will be helpful for readers.

ye-luo · 2020-01-04T08:48:35Z

src/QMCHamiltonians/EwaldTools.h

+namespace qmcplusplus
+{
+
+  using namespace ewaldref;


"using namespace" in header files may propagate very far away. I think all the functions and classes can be put inside the ewaldref name space.

When adding new code to the source, how do you determine what needs separate namspace(s) and what does not? Most of the code simply falls within the qmcplusplus namespace (which really should be qmcpack since the name of the code changed from QMC++ long ago...).

There are two topics.

header file "using namespace" is a bad practice. cpp file is fine. "using namespace" propagates in header files but not in cpp files.

There is no strict rule whether extra namespace should be used or not. The original EwaldRef.h defines a few datatypes directly in the qmcplusplus name space. They can propagate and can be misused or cause potential name collisions. So I put most of the content of EwaldRef.h in ewaldref namespace within qmcplusplus namespace as a protection. Here the added code is quite standalone and closely related to EwaldRef then putting it in the same namespace as EwaldRef seems appropriate.

I'm fine with this, though the new code is not going to be used as reference. If it has a future, it will be in the main application, otherwise it will be deleted.

If you intend to implement a class derived from OperatorBase, then don't put the class in ewaldref namespace. Instead, you can either put ewaldref:: for using any piece from EwaldRef or "using ewaldref::ABC" in the class to bring in ABC to the new class.

ye-luo · 2020-01-04T08:50:19Z

src/QMCHamiltonians/EwaldRef.h

@@ -416,7 +416,7 @@ real_t ewaldEnergy(const RealMat& a, const PosArray& R, const ChargeArray& Q, re

 #pragma omp parallel for reduction(+ : ve)
    for(size_t n=0; n<Npairs; ++n)
-      ve += qq[n]*ewaldSum(rr[n],a,tol/qq[n]);
+      ve += qq[n]*ewaldSum(rr[n],a,tol/qq[n]/Npairs);


If possible, please run clang-format on both files.

Not time efficient. I'm waiting for the automatic part of #794 to be implemented.

The automatic part has been implemented; see https://github.com/QMCPACK/qmcpack/wiki/Source-formatting

This was a decision made as a project to not have a robot do formatting without supervision, but to provide various methods for different workflow preferences that everyone can choose for themselves, from assisted manual to full automatic.

ye-luo · 2020-01-04T08:56:08Z

src/QMCHamiltonians/EwaldTools.h

+  }
+
+
+  real_t madelungConstant()


Please revise function names in this file following our function naming rules. For example here, calculateMadelungConstant(). This function also doesn't modify any class member, so please mark this function const.

The naming already follows the coding conventions, as in the manual:

27.3.5 (Member) function names Function names should start with a lowercase character and have a capital letter for each new word.

Adding calculate to every function that calculates something is IMO trite and lexically wasteful.

Will add const.

madelungConstant() is not clear whether it to access a stored value in the class or run a calculation. When I name functions, evaluate/calculateXXX() means it can be costly. getXXX() means cheap. Probably in your case, it doesn't matter much.

ye-luo · 2020-01-04T09:02:13Z

src/QMCHamiltonians/EwaldTools.h

+  int_t km   = 0;
+  IntVec iv;
+  // Add the value at the origin, if requested.
+  if (zero)


if we define a member function addZeroContribution() to both T& function, there is no need of the zero switch.

The term should not need to know how it is being summed.

ignore my last comment. zero and function need to be controlled independently.

ye-luo · 2020-01-06T01:47:05Z

~~Leave some notes of the understanding from my last optimization attempt.~~
The k-space term is expensive when calculation sincos in exp(G (Ri-Rj)), a direct computation cost Ng x Natom x Natom. We can separate this term into exp(Gs Ri) x exp(-Gs Rj) and the cost reduces to 2 x Ng x Natom.
~~The r-space term is expensive in erfc(|Rs -(Ri - Rj)|). It costs Nr x Natom x Natom. I don't have a way to reduce its scaling.~~
~~The kappa choice we have in the current code assumes the same scaling for both K and R space computation. I saw more or less the same time spent in both parts.~~
~~To have a fully balanced K and R space computation, we need to take into account~~
~~1. the single function call cost of sincos() and erfc()~~
~~2. the scaling difference between K and R space computation if we use the separate form to reduce computation.~~
moved to #2176 which is more relavent.

@jtkrogel
Q. How was the performance comparison made? just Ion-ion coulomb calculation or electron-electron term as well?
I misunderstood "outperform" for computational cost.

jtkrogel · 2020-01-06T15:10:39Z

I'm not sure what you are asking for in terms of my documenting your last optimization attempt. This code is not presently visible, and optimizations of that type are already well documented in the manual, e.g. in equations 26.11-26.13.

The code in this PR is unoptimized as yet. When I mentioned "outperformed" above, I meant in terms of accuracy. I measured the potential for similar efficiency based on the number of points that need to be summed (which is about equivalent) for the ion-ion sum.

In the next step (after this PR), I will attempt to replace QMCPACK's use of the opt breakup with this code for the e-e coulomb part and compare speed. Optimization may follow at that point (using OMP, inlining and reordering sums, etc).

jtkrogel · 2020-01-06T15:54:34Z

I've added new namespace scoping, the requested const function and light function documentation.

I'm willing to improve these further in the later event that this code is adapted and used in the main application. For now, the code is exploratory. Good enough for now?

ye-luo

LGTM

grahamlopez · 2020-01-06T16:19:28Z

I'm willing to improve these further in the later event that this code is adapted and used in the main application. For now, the code is exploratory. Good enough for now?

Please be careful with this kind of thinking. These PR's are the main gate to code getting merged into develop. Once in develop, intentional steps have to be taken by a human to keep it from eventually going into production. Of course intentions can be good, but we know how plans/priorities change, tasks shift to different people, code gets forgotten/neglected, etc. If this code works well enough to be merged now, why shouldn't it deserve the same kind of care as any other? The amount of work is the same (or probably less) if it happens now especially since it currently has attention vs. when it possibly gets revisited later for separate reasons.

prckent

This code should not be merged until we have had a full discussion around the criteria for updating the current Coulomb code.

Adding some comments here is on my to do list for today.

jtkrogel · 2020-01-06T16:32:33Z

N.B. This code is not used anywhere outside of its own header file right now.

I'm happy to retract this PR and keep on exploring, but I thought making a waypoint for everyone else to see would be useful.

jtkrogel · 2020-01-06T16:39:29Z

I'll make an issue elaborating where I am in the exploration process and where it might lead in later steps.

jtkrogel · 2020-01-06T16:58:02Z

See #2185.

jtkrogel · 2020-01-09T15:11:58Z

Will update the code here occasionally as #2185 unfolds.

add anisotropic ewald code

5a0ff83

remove commented code

5415831

jtkrogel changed the title ~~Anisotropic ewald code~~ Anisotropic Ewald code Jan 3, 2020

fix typos in comments

1c927f0

ye-luo reviewed Jan 4, 2020

View reviewed changes

add namespace, const, and light function docs

fd926a2

ye-luo previously approved these changes Jan 6, 2020

View reviewed changes

prckent self-requested a review January 6, 2020 16:27

prckent requested changes Jan 6, 2020

View reviewed changes

jtkrogel mentioned this pull request Jan 6, 2020

Anistropic grids for Ewald Coulomb evaluation #2185

Open

3 tasks

jtkrogel added 2 commits January 6, 2020 12:54

split off cpp file for EwaldRef

f9db5f4

comparing aniso and qmcpack ewald

8b06e0a

prckent mentioned this pull request Jan 7, 2020

Add XML selection of LR Handler #2190

Merged

jtkrogel added 3 commits January 7, 2020 16:12

merge with develop

00ef7f7

start optimized ewald

51a77f7

cleanup state machine

634e8c7

jtkrogel changed the title ~~Anisotropic Ewald code~~ [WIP] Anisotropic Ewald code Jan 9, 2020

jtkrogel added 4 commits January 9, 2020 10:13

rename functions

1aa8058

add SR and LR error bounds

9162af3

add search to enforce bounds

3ef40f0

enable accuracy vs time comparisons

d55eeba

prckent mentioned this pull request Mar 6, 2020

adding qmcfinitesize tool #2329

Merged

prckent dismissed ye-luo’s stale review via d55eeba July 26, 2021 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Anisotropic Ewald code #2182

[WIP] Anisotropic Ewald code #2182

jtkrogel commented Jan 3, 2020

qmc-robot commented Jan 3, 2020

ye-luo left a comment

ye-luo Jan 4, 2020

jtkrogel Jan 6, 2020

ye-luo Jan 6, 2020

jtkrogel Jan 6, 2020

ye-luo Jan 6, 2020

ye-luo Jan 4, 2020

jtkrogel Jan 6, 2020

grahamlopez Jan 6, 2020

ye-luo Jan 4, 2020

jtkrogel Jan 6, 2020

ye-luo Jan 6, 2020

ye-luo Jan 4, 2020

jtkrogel Jan 6, 2020

ye-luo Jan 6, 2020

ye-luo commented Jan 6, 2020 •

edited

Loading

jtkrogel commented Jan 6, 2020

jtkrogel commented Jan 6, 2020

ye-luo left a comment

grahamlopez commented Jan 6, 2020

prckent left a comment •

edited

Loading

jtkrogel commented Jan 6, 2020

jtkrogel commented Jan 6, 2020

jtkrogel commented Jan 6, 2020

jtkrogel commented Jan 9, 2020

[WIP] Anisotropic Ewald code #2182

Are you sure you want to change the base?

[WIP] Anisotropic Ewald code #2182

Conversation

jtkrogel commented Jan 3, 2020

qmc-robot commented Jan 3, 2020

ye-luo 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

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

ye-luo commented Jan 6, 2020 • edited Loading

jtkrogel commented Jan 6, 2020

jtkrogel commented Jan 6, 2020

ye-luo left a comment

Choose a reason for hiding this comment

grahamlopez commented Jan 6, 2020

prckent left a comment • edited Loading

Choose a reason for hiding this comment

jtkrogel commented Jan 6, 2020

jtkrogel commented Jan 6, 2020

jtkrogel commented Jan 6, 2020

jtkrogel commented Jan 9, 2020

ye-luo commented Jan 6, 2020 •

edited

Loading

prckent left a comment •

edited

Loading