-
Notifications
You must be signed in to change notification settings - Fork 7
Update Ewald error formula #198
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs should also be updated with the change.
src/torchpme/tuning/ewald.py
Outdated
| / torch.sqrt(2 * torch.pi**2 * self.volume / lr_wavelength) | ||
| * torch.exp(-2 * torch.pi**2 * smearing**2 / lr_wavelength**2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this change is correct? I think units still don't match because smearing and the lr_wavelength should cancel. I just re-derived the equation and I got
maybe check it again.
I think we should also give the conversion between our convention and the other usual one somewhere in the docs:
where α is the inverse width of Gaussian charge cloud and
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix @GardevoirX!
52d8dab to
7edf39f
Compare
7edf39f to
58a3ce2
Compare
Fixes the error formula of Ewald used in tuning. The fixed formula is tested with systems of crystal cells repeated 8 times on each dimension.
No difference in the actual accuracy is observed.

The Ewald calculation time is a bit longer with the fixed formula.

Contributor (creator of pull-request) checklist
Reviewer checklist
📚 Documentation preview 📚: https://torch-pme--198.org.readthedocs.build/en/198/