-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Support for tiled decompositions in PPPM #2280
Conversation
@akohlmey @stanmoore1 As we discussed this PR initially has new PPPM2 and GridComm2 classes with the added support for tiling. The original PPPM and GridComm classes are unchanged. |
@akohlmey Generalized this so the GridComm class can be used by fixes or computes. Similar to how the Comm methods for forward/reverse comm can be called from pair, fix, compute styles. Maybe this would be better done with a single templated forward_comm() method in GridComm2 which was templated on a ptr to a KSpace, Fix, Compute instance ? |
@sjplimp ran this with the unittest test programs and found no difference between when switching to comm_style tiled, but those are all serial tests, so I am not sure how relevant those are. What worries me a little bit is that changing from orthogonal to triclinic changes the forces even though all the tilt factors are 0. This is the same for pppm an pppm2 though. |
Are you manually constraining the grid? I believe the logic for choosing the grid size is a little different for triclinic vs not. |
nope. there are the test inputs and lists of reference forces/energies/stresses: https://github.com/lammps/lammps/blob/master/unittest/force-styles/tests/kspace-pppm.yaml and https://github.com/lammps/lammps/blob/master/unittest/force-styles/tests/kspace-pppm_tri.yaml all kspace related commands are in the "post_commands" section. The tester feeds input fragments, files and settings from these files into a LAMMPS instance and then compares the forces, stresses and energies. |
You will get slightly different forces if triclinic picks say a 34x34x34 grid while orthogonal picks a 32x32x32 grid. |
@akohlmey @stanmoore1 Did this difference between orthogonal and triclinic for an orthog geometry get resolved? This was for both the original and this new PPPM2, so not an issue caused by this PR, correct? I am about ready to convert this PPPM2 to PPPM and propagate the changes to other files. Any reason to still wait on doing that? |
@akohlmey @stanmoore1 I believe I have made all the needed mods to all the PPPM variants, except Kokkos. I also removed the trial PPPM2 and GridComm2 classes. So now the changes are in the mainstream PPPM and GridComm class. I think more changes are needed in GridCommKokkos than in PPPMKokkos. MSM is more complicated. I think we should disable tiled with MSM for now, but it will take some changes to MSM to make it work with the new GridComm API. If you can test all the PPPM variants, w/out MSM in a good state, please do. |
@akohlmey @stanmoore1 ok, with Stan's advice I made MSM compatible with the new GridComm class. So everything but PPPM/Kokkos is now in place. Please test everything you can think of. |
@sjplimp While I look into this, I need to you to look at PR #2284 |
@sjplimp this branch current fails to compile kspace style msm/cg/omp.
In both cases initializing the affected pointers to NULL in the constructor resolved those issues. This is done in commit bd79179 after that the unittest result is:
So only this and the failed compilation of msm/cg/omp remains:
|
@akohlmey I suppose everything is good, and there's no need for my review at this point. |
yes, your review was auto-requested because you are listed as a code owner for one of the affected files in |
@sjplimp i've resolved the merge conflict and added a couple of unit tests for kspace with comm_style tiled (won't really test the communication patterns, but will help us detect when changes have side effects). I will now trigger the full regression tests one more time. Anything else that needs to be done before this is ready to merge? |
We figured out how to get correct forces when flipping FFT forward/reverse (that @sjplimp tried to add). Do we just want to make that a new PR and add that later? |
Yes. You could make a feature branch from this branch and then submit this as PR. If you compile with cmake and enable testing, you can then test the kspace styles selectively with |
@akohlmey @stanmoore1 I'm a bit lost in the git lingo. If you're saying you want to (a) drop any FFT changes from this PR and test/merge it now, and then (b) do another PR later to make the FFT and PPPM changes, then that's fine by me. |
yes. All I was saying is that it is possible to create the branch with I am currently trying to identify some issue that was flagged by one of the load balancing regression tests but cannot (yet) reproduce it. |
Summary
Add support to PPPM for load-balanced decompositions produced by balance and fix balance rcb,
i.e. tiled decompositions. The majority of changes for this are in the GridComm class which now stores a regular or tiled decomposition of the FFT grids used by PPPM and does forward and reverse communication of the charge and efield values on those grids. This change should support both orthogonal and triclinic domains and all the options that the PPPM class supports. By the time it is ready to merge all variant PPPM classes, including accelerator versions, should be supported.
Related Issues
N/A
Author(s)
Steve
Licensing
By submitting this pull request, I agree, that my contribution will be included in LAMMPS and redistributed under either the GNU General Public License version 2 (GPL v2) or the GNU Lesser General Public License version 2.1 (LGPL v2.1).
Backward Compatibility
Non-tiled decompositions should work as before.
Implementation Notes
Post Submission Checklist
Further Information, Files, and Links