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

Use minimum image of particles in EwaldRef #3763

Merged
merged 5 commits into from
Jan 26, 2022
Merged

Conversation

ye-luo
Copy link
Contributor

@ye-luo ye-luo commented Jan 25, 2022

Proposed changes

The reference calculation went wrong this time.
When particles don't start within the unit cell, the grid summation of EwardRef doesn't work well. See first commit.
The QMCPACK value is OK. #2105 (comment) passes with this fix.

Closes #2105

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

  • Bugfix

Does this introduce a breaking change?

  • No

What systems has this change been tested on?

epyc-server

Checklist

  • Yes. This PR is up to date with current the current state of 'develop'

@ye-luo
Copy link
Contributor Author

ye-luo commented Jan 25, 2022

Test this please

@prckent prckent requested a review from jtkrogel January 25, 2022 14:15
@prckent prckent changed the title EwardRef better to start with particles in the cell. Use minimum image of particles in EwardRef Jan 25, 2022
@prckent prckent changed the title Use minimum image of particles in EwardRef Use minimum image of particles in EwaldRef Jan 25, 2022
@jtkrogel
Copy link
Contributor

Is the underlying issue a premature truncation of the real space sum?

If r_i-r_j is outside the cell, then the first term in the sum (centered at zero lattice vector shift) could be rather small, perhaps smaller than the tolerance for the sum overall.

@ye-luo
Copy link
Contributor Author

ye-luo commented Jan 25, 2022

Is the underlying issue a premature truncation of the real space sum?

Yes

If r_i-r_j is outside the cell, then the first term in the sum (centered at zero lattice vector shift) could be rather small, perhaps smaller than the tolerance for the sum overall.

The tolerance check is on the sum of the absolute values contributed by grid points on the surface shell.
This first term (all 0 offset) is always excluded from the tolerance check. Instead some later shell went under tolerance before reaching the shells which contributes significant values when the initial coordinate delta are very far from the center.

@prckent
Copy link
Contributor

prckent commented Jan 25, 2022

I also thought about testing for several shells in a row being below tolerance, but I think once the particles are minimum imaged that is no longer necessary. Also, if a particle ever wandered "far enough" there could still be an issue with not testing enough shells.

Since this fixes the reported problem, adds a test for it, and doesn't require any changes to the values used in other tests, can we consider this safe to merge?

@ye-luo
Copy link
Contributor Author

ye-luo commented Jan 25, 2022

Test this please

Copy link
Contributor

@jtkrogel jtkrogel left a comment

Choose a reason for hiding this comment

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

Yes, let's merge this.

@ye-luo
Copy link
Contributor Author

ye-luo commented Jan 26, 2022

Test this please

@ye-luo ye-luo enabled auto-merge January 26, 2022 18:39
@ye-luo ye-luo merged commit fea03c5 into QMCPACK:develop Jan 26, 2022
@ye-luo ye-luo deleted the fix-ewardref branch January 29, 2022 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect ion-ion energy in some periodic systems
3 participants