-
Notifications
You must be signed in to change notification settings - Fork 139
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
Resurrect Ewald2D #3852
Resurrect Ewald2D #3852
Conversation
Paul - Thanks for looking at this. It would be good to have a proper 2D evaluation... if this is a proper implementation. How thoroughly have you checked this code and its outputs? I think it is safe to say that shortcuts were made in the historical Coulomb code, so before we can advertise 2D Coulomb as working and add examples etc. I think we need to be very thorough with testing, convergence checks, validation etc. |
@prckent I looked at Madelung energy of square and triangular lattices using the unit cell and select supercells. That said, I'm not comfortable with this becoming a production-level feature right now. For now, this feature can be enable only via an undocumented flag |
|
I'll need to figure out how to add these to the unit test. |
@prckent I added all my checks to the unit test. I'm not sure why the CI cannot build the code. |
I would like to discuss a little about the use of If so, I don't think we should use As a follow-on, does the current code support ppn/pnp/npp or just ppn? |
|
@jtkrogel Yes, Ewald2D is meant for a system that's periodic only in 2 directions. I don't think we can have |
Could you explain more? I don't understand the distinction (2D/quasi-2D) if all systems in question exist in a 3D space. |
By true 2D, I mean that all particles stay exactly on the same plane. |
No, I do not have a 3D input with expanding z direction. |
On second thought, 3D ppn with large z will not match the quasi 2D value because they have different neutralizing backgrounds. |
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.
It looks like my previous review was addressed where possible for this PR except for this.
// make sure we can ignore the short-range Madelung sum | ||
mRealType Rws = Ps.getLattice().WignerSeitzRadius; | ||
mRealType rvsr_at_image = Rws*AA->evaluate(Rws, 1.0/Rws); | ||
if (rvsr_at_image > 1e-6) |
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.
If you want behavior to be preserved it should be tested, sometimes you need to be as clever in test code as you are in the actual implementation. You should be able to devise a synthetic case where this is true.
From the standpoint of the exception, putting the failing value in the app_log and it not actually being the alpha value means digging back into the implementation of the handlers for anyone that runs into this. Use a string stream or some other method and get the bad value into the exception's what string. You might need to indicate what the relationship between the short range part of the ewald being > 1e-6 at the WSR and apha being too small. Is this message valid for other choices of LRHANDLER? That is what the terribly name AA is right?
Could you also document the new option value in the manual? |
@PDoakORNL I clarified the exception message and added a test with CHECK_THROWS. |
@ye-luo I added documentation for |
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.
Only minor things left.
Not directly related to this PR. I noticed quasi2D doesn't seem to be connected in PBCAB.
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.
LGTM
Test this please |
@PDoakORNL OK to merge? |
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.
It think that this meets easily meet the merge criteria now, and it can clearly be refined further as/if needed. Thanks for enduring the review Paul. I think that the code is better for it.
Keep things moving. Can be revisited in future if not considered sufficiently addressed.
@Paul-St-Young satsified my important concerns. |
Please review the developer documentation
on the wiki of this project that contains help and requirements.
Proposed changes
Resurrect true 2D Ewald sum without using
OHMMS_DIM
.What type(s) of changes does this code introduce?
Does this introduce a breaking change?
What systems has this change been tested on?
Workstation
Checklist
Update the following with a yes where the items apply. If you're unsure about any of them, don't hesitate to ask. This is
simply a reminder of what we are going to look for before merging your code.