Allow setting the number of grid points in the short range eward summation of Coulomb interaction by Paul-St-Young · Pull Request #4928 · QMCPACK/qmcpack

Paul-St-Young · 2024-02-15T21:05:00Z

Proposed changes

Avoid instantiation of LinearGrid object at call time in createSpline4RbyVs*, because it silently defaults to 1001 grid points without user control.
Instead, I added a new ewald_grid input to set the density of the linear grid, which is used to interpolate the short-range part of the Ewald Coulomb interaction.

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

Refactoring

Does this introduce a breaking change?

No

What systems has this change been tested on?

Intel 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
No. This PR adds tests to cover any new code, or to catch a bug that is being fixed
No. Documentation has been added (if appropriate)

prckent · 2024-02-15T21:31:58Z

Test this please

ye-luo

Since there are many pointer manipulation, I will need to review it more closely. For now, could you put up a manual entry explaining what is "ewald_grid"? It is ewald related but what grid it is and how it's been used?

ye-luo

I'm still going through CoulombPBCAB about how grids are managed. Please address other comment.

ye-luo · 2024-02-17T00:36:26Z

  +---------------------+--------------+---------------------------+-------------------+----------------------------------------------------+
  | ``LR_tol``          | float        | float                     | 3e-4              | Tolerance in Ha for Ewald ion-ion energy per atom. |
  +---------------------+--------------+---------------------------+-------------------+----------------------------------------------------+
+  | ``ewald_grid``      | int          | int                       | 1001              | Linear grid used for short-range part of Ewald.    |


"The number of linear grid points used for ..."

ye-luo · 2024-02-17T00:37:45Z

  ///number of strictly enforced periodic spatial dimensions
  /// ewald_strict2d sets ndim=2, otherwise ndim=3
  unsigned ndim;
+  unsigned ewaldGrid;


In the source code, we better to use a clear name for the variable. "ewald_num_grid_points". Also please add a line of documentation.

ye-luo · 2024-02-17T00:42:39Z

+  ///cutoff radius of the short-range part
  RealType myRcut;
+  ///radial grid
+  std::shared_ptr<GridType> myGrid;


Just define it locally in initBreakup. I don't even think we need a pointer.

ye-luo

@Paul-St-Young please address my review when you get a chance.

ye-luo · 2024-02-22T21:54:50Z

  RealType h; //this is for finite differencing.
  std::vector<PosType> sphericalgrid;
-  GridType* myGrid;
+  std::shared_ptr<GridType> myGrid;


I don't see why you choose shared_ptr. Should be "unique_ptr"

Paul-St-Young · 2024-02-23T00:51:12Z

@ye-luo thanks for the great suggestions!
Indeed, no pointer to the LinearGrid is needed in any of the cases I touched.

ye-luo · 2024-02-23T17:03:05Z

Test this please

ye-luo · 2024-02-23T17:05:17Z

@Paul-St-Young did you play with the number of grid points and what was your finding?

ye-luo · 2024-02-23T01:55:47Z

  +---------------------+--------------+---------------------------+-------------------+----------------------------------------------------+
  | ``LR_tol``          | float        | float                     | 3e-4              | Tolerance in Ha for Ewald ion-ion energy per atom. |
  +---------------------+--------------+---------------------------+-------------------+----------------------------------------------------+
+  | ``ewald_grid``      | int          | int                       | 1001              | The number of linear grid points used for short-range part of the Ewald potential. |


There is formatting issue causing the table not being rendered.

Took care of.

ye-luo · 2024-02-23T01:56:14Z

    Base::LR_dim_cutoff = rhs.LR_dim_cutoff;
    Base::LR_kc         = rhs.LR_kc;
    Base::LR_rc         = rhs.LR_rc;
+    Base::num_ewald_grid_points     = rhs.num_ewald_grid_points;


Run clang-format please.

Took care of.

prckent · 2024-02-23T19:29:52Z

Very happy to see the exposure of this hidden default. Thanks Paul

Weirdly the estimator unit test failed in the complex coverage test. Will retry it.

ye-luo · 2024-02-23T20:30:36Z

Very happy to see the exposure of this hidden default. Thanks Paul

Weirdly the estimator unit test failed in the complex coverage test. Will retry it.

Still failing @PDoakORNL any clue? https://github.com/QMCPACK/qmcpack/actions/runs/8022503383/job/21922019273?pr=4928

Paul-St-Young · 2024-02-24T12:51:45Z

@ye-luo Yes, I tried different number of grid points for both the bare and screened Coulomb potential.
In the bare Coulomb case, 1001 grid points has never been a problem.
However, much denser grid is needed for the screened potential in a large supercell, because it falls off more quickly than the bare Coulomb. For example, if the screened potential vanishes at Rcut/10, then effectively its using only 100 grid points. One can also imagine using a more wiggly effective e-e interaction, which would require more grid points to represent accurately.

Short answer: the number of grid points may need adjustment if people want to use anything other than 1/r interaction.

ye-luo · 2024-03-04T20:43:35Z

Test this please

Paul-St-Young added 9 commits February 9, 2024 16:43

remove agrid_local

5c3d98b

add ewald_grid as input

46dfcd6

initialize QMCFiniteSize::myGrid

d3d107e

reorder ndim ewaldGrid

cd6a948

initialize myGrid in CoulombPBCAB

270cfca

require agrid input to createSpline

4380cce

tweak message

d121ade

Merge remote-tracking branch 'upstream/develop' into vr_spline_grid

4dff0a1

fix typo in test input

198a4ed

Paul-St-Young added the refactor label Feb 15, 2024

Paul-St-Young requested a review from ye-luo February 15, 2024 21:05

Paul-St-Young self-assigned this Feb 15, 2024

ye-luo reviewed Feb 15, 2024

View reviewed changes

document ewald_grid input

6015367

ye-luo reviewed Feb 17, 2024

View reviewed changes

ye-luo reviewed Feb 22, 2024

View reviewed changes

Paul-St-Young added 6 commits February 22, 2024 19:18

tweak ewald_grid doc

7706fbf

rename ewaldGrid to num_ewald_grid_points

9b877b4

pass LinearGrid by reference instead of as pointer

e58afe7

define LinearGrid locally rather than as pointers

d24f6b8

make agrid_local local

2119d03

Merge remote-tracking branch 'upstream/develop' into vr_spline_grid

514596c

ye-luo added 4 commits February 22, 2024 20:22

Minor adjustment.

e4b3c0f

Formatting.

f22213b

Merge remote-tracking branch 'origin/develop' into pr/origin-4928

2711072

Increase timeout retry.

a4e1fae

ye-luo changed the title ~~forbid nullptr as LinearGrid input to createSpline4RbyVs~~ Allow setting the number of grid points in the short range eward summation of Coulomb interaction Feb 23, 2024

ye-luo previously approved these changes Feb 23, 2024

View reviewed changes

ye-luo enabled auto-merge February 23, 2024 17:06

ye-luo dismissed their stale review via d0ff0c1 March 4, 2024 16:56

Resolve uninitialized value issue.

1aa5148

ye-luo force-pushed the vr_spline_grid branch from d0ff0c1 to 1aa5148 Compare March 4, 2024 17:18

Merge branch 'develop' into vr_spline_grid

5706758

ye-luo approved these changes Mar 4, 2024

View reviewed changes

ye-luo merged commit fc5089c into QMCPACK:develop Mar 4, 2024

Paul-St-Young deleted the vr_spline_grid branch March 13, 2024 15:45

Conversation

Paul-St-Young commented Feb 15, 2024

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

Uh oh!

prckent commented Feb 15, 2024

Uh oh!

ye-luo left a comment

Choose a reason for hiding this comment

Uh oh!

ye-luo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ye-luo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Paul-St-Young commented Feb 23, 2024

Uh oh!

ye-luo commented Feb 23, 2024

Uh oh!

ye-luo commented Feb 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prckent commented Feb 23, 2024

Uh oh!

ye-luo commented Feb 23, 2024

Uh oh!

Paul-St-Young commented Feb 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ye-luo commented Mar 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Paul-St-Young commented Feb 24, 2024 •

edited

Loading