Fix electrode charge sync for split K-space - #8
Open
HanswithCMY wants to merge 2 commits into
Open
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoSynchronize dynamic electrode charges before split K-space computation
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can reply 'qodo' on any finding to push back, ask questions, or dig deeper |
There was a problem hiding this comment.
🟢 Approval recommended
The functional change is straightforward and localized to the intended communication path, with only a minor comment typo to address.
Pull request overview
This PR fixes a correctness issue in split R-space/K-space runs where K-space PPPM could use stale electrode charges by synchronizing atom->q from R-space to K-space every timestep, after PRE_FORCE (when electrode fixes update charges).
Changes:
- Move charge transfer from one-time
rk_setup()logic to per-stepVerletSplitKSpace::r2k_comm(). - Synchronize coordinates and charges together via
MPI_Scatterv()inr2k_comm()to ensure consistent electrostatic state on K-space ranks. - Leave the old
rk_setup()charge scatter as commented “archived” code with an explanatory note.
File summaries
| File | Description |
|---|---|
src/lmp/verlet_split_kspace.cpp |
Adds per-step R→K scatter of atom->q (aligned with atom->x scatter) and updates comments to reflect new synchronization point. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // MPI_Scatterv(master ? atom->q : nullptr, qsize, qdisp, MPI_DOUBLE, | ||
| // atom->q, n, MPI_DOUBLE, 0, block); | ||
| // Disabled because electrode fixes may update atom->q every PRE_FORCE; | ||
| // transferring only at reneighbouring leaves K-space PPPM with stale charges. |
❌ 3 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
There was a problem hiding this comment.
🟢 Approval recommended
The change is localized and correctly synchronizes atom->q in the same collective communication phase as coordinates, matching the intended timing after PRE_FORCE and before K-space computation.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix dynamic electrode charge synchronization in
verlet/split/kspaceandverlet/split/dplrProblem
verlet/split/dplrseparates R-space and K-space into distinct MPIpartitions. The previous implementation transferred
atom->qfrom R-space toK-space only in
rk_setup(), which runs at initialization and after neighborlist rebuilding.
This is incorrect for dynamic electrode fixes such as
electrode/conqandelectrode/conp: they update electrode charges at every MD step duringPRE_FORCE. Consequently, R-space pair interactions used the current charges,whereas K-space PPPM could use charges from the previous neighbor rebuild.
Fix
Move charge synchronization to
VerletSplitKSpace::r2k_comm(), which is calledafter
PRE_FORCEand before the K-space calculation. Coordinates and chargesare now transferred together every step.
The old
rk_setup()charge transfer is retained as commented archived code todocument the previous behavior and its limitation.
Validation
A direct R-space/K-space charge trace was recorded for a 30-step
electrode/conqtrajectory withneigh_modify every 10.sum(q^2)was piecewise constant and updatedonly after neighbor rebuilding (every 10 steps in this case). At each such
transfer, its charge state was one integration step behind the current
R-space state, because the transfer occurred before the subsequent
PRE_FORCEcharge update. The maximum per-atom R/K charge mismatch was1.46 e.maximum per-atom mismatch is zero to printed precision.