feat(lammps): add compact DeePMD evaluation - #5943
Open
njzjz wants to merge 7 commits into
Open
Conversation
Select a periodic cutoff environment around a center group, optionally promote cutoff hits to complete molecules, and mark excluded model atoms with type -1 so existing DeepPot compaction handles inference and output scattering. Synchronize selection across MPI ranks, invalidate cached neighbor mappings when it changes, normalize model deviation over selected atoms, document the interface, and add serial, triclinic, cache-transition, and MPI coverage. Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded cutoff-based compact atom selection to ChangesCompact DeepMD evaluation
Estimated code review effort: 4 (Complex) | ~50 minutes Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
source/lmp/pair_deepmd.cpp (1)
303-328: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the neutral-element contract for the no-selection case.
If no owned atom is selected, this function leaves
max,min, andsumat the caller's initial values. The single caller setsmin = numeric_limits<double>::max(),max = 0, andavg = 0before the call, so the followingMPI_Reduceoperations stay correct. That contract is implicit. Add a short comment so a future caller does not initialize the outputs differently and produce a wrong global minimum.♻️ Proposed comment
+ // No owned atom is selected on this rank. Leave the caller's initial values + // untouched; the caller must seed them with the reduction-neutral elements + // (min = DBL_MAX, max = 0, sum = 0) so the MPI reductions stay correct. bool found = false; for (int ii = 0; ii < nlocal; ++ii) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/lmp/pair_deepmd.cpp` around lines 303 - 328, Add a concise comment in PairDeepMD::analyze_model_deviation documenting that when no owned atoms are selected, max, min, and sum must remain unchanged so the caller-provided neutral values remain valid for subsequent MPI_Reduce operations. Do not alter the selection or accumulation logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/lmp/tests/test_lammps_compact.py`:
- Around line 195-201: Update the sp.run invocation in the MPI test to include
an explicit timeout=60, matching the sibling test’s subprocess behavior. Keep
the existing command, working directory, output capture, and return-code
assertion unchanged.
---
Nitpick comments:
In `@source/lmp/pair_deepmd.cpp`:
- Around line 303-328: Add a concise comment in
PairDeepMD::analyze_model_deviation documenting that when no owned atoms are
selected, max, min, and sum must remain unchanged so the caller-provided neutral
values remain valid for subsequent MPI_Reduce operations. Do not alter the
selection or accumulation logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a1d13ff2-7727-4980-a727-6b4d17a96c57
📒 Files selected for processing (6)
doc/third-party/lammps-command.mdsource/lmp/pair_deepmd.cppsource/lmp/pair_deepmd.hsource/lmp/pair_deepmd_kokkos.cppsource/lmp/tests/test_lammps_compact.pysource/lmp/tests/test_lammps_option_parsers.py
Keep collective send scalars mutable because the LAMMPS serial MPI stubs expose pre-const-correct void pointer parameters. Also document the model-deviation reduction seeds and bound MPI compact tests with an explicit timeout. Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5943 +/- ##
==========================================
- Coverage 79.41% 77.56% -1.86%
==========================================
Files 1072 1072
Lines 124893 125410 +517
Branches 4531 4617 +86
==========================================
- Hits 99187 97268 -1919
- Misses 24085 26746 +2661
+ Partials 1621 1396 -225 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Only expose the DeepMD cutoff through cut_coul for pppm/dplr. This allows ordinary long-range Coulomb sub-styles to use a different cutoff under hybrid/overlay without breaking the existing DPLR split. Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
Walk the DeepMD full-neighbor rows for center atoms instead of scanning every center against the full local-plus-ghost atom set on each step. Cache static center membership between neighbor rebuilds while retaining cutoff-crossing behavior. Recover zero-valued special_bonds exclusions through a deferred tag lookup so compact selection remains independent of force-field exclusions. Add regression coverage for the excluded bonded-pair case. Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (3)
source/lmp/pair_deepmd.cpp:1032
- PairDeepMD::settings resets compact-selection state but leaves compact_is_center_ intact. If pair_style deepmd is redefined (or settings re-run) without forcing a neighbor rebuild, apply_compact_selection() can reuse a same-sized compact_is_center_ cache computed for a previous center_group, producing an incorrect center/environment selection. Clear compact_is_center_ alongside the other compact-selection caches during settings().
compact_natoms_ = 0;
compact_center_group_id_.clear();
compact_center_tags_.clear();
compact_selected_.clear();
source/lmp/pair_base.cpp:588
- PairDeepBaseModel::extract returns nullptr for "cut_coul" when not using pppm/dplr, but in that branch it does not set the out-parameter dim. Leaving output parameters uninitialized can cause undefined behavior if a caller reads dim regardless of the returned pointer. Set dim before returning nullptr for consistency with other branches.
if (force->kspace_style == nullptr ||
strcmp(force->kspace_style, "pppm/dplr") != 0) {
return nullptr;
}
source/lmp/pair_deepmd_kokkos.cpp:183
- In pair_style deepmd/kk, the unsupported-compact-evaluation diagnostic is checked only after calling PairDeepMD::init_style(). If compact options are set but another compact-related precondition fails first (e.g., missing group, molecule IDs, or atom IDs), users may never see the intended "not supported" message. Check compact_selection_enabled() before calling PairDeepMD::init_style() so deepmd/kk always diagnoses this mode as unsupported.
// Base setup and the full neighbor-list request.
PairDeepMD::init_style();
if (compact_selection_enabled()) {
error->all(FLERR,
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Express the create_box command as one string so CodeQL does not flag implicit list-item concatenation. Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
source/lmp/pair_deepmd.cpp (1)
296-306: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate tags in
tag_to_indexcan map to a far periodic image.
tag[jj]repeats for each ghost image of the same atom.emplacekeeps the first insertion, sofind_atom_by_tagcan return a distant image index. The followingselect_environment_atomcall passesapply_minimum_image = true, so the distance test recovers the correct value. The selected index is still an arbitrary image.The current use is limited to the distance test and to
tag[environment]ormolecule[environment], and both are image-invariant. Add a short comment that states this requirement, so a later change does not read image-dependent data from the returned index.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/lmp/pair_deepmd.cpp` around lines 296 - 306, The tag lookup may return an arbitrary periodic image when duplicate ghost tags exist. Add a short comment adjacent to find_atom_by_tag or its use documenting that the returned index is valid only for image-invariant data such as distance checks, tag, and molecule, and must not be used to read image-dependent properties.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/lmp/pair_deepmd.cpp`:
- Around line 290-332: Update init_style() to reject active neigh_modify exclude
type/group/molecule settings before compact selection is used, covering all
neighbor-exclusion modes that can remove environment pairs. Keep the existing
special_bonds recovery in apply_compact_selection() unchanged.
---
Nitpick comments:
In `@source/lmp/pair_deepmd.cpp`:
- Around line 296-306: The tag lookup may return an arbitrary periodic image
when duplicate ghost tags exist. Add a short comment adjacent to
find_atom_by_tag or its use documenting that the returned index is valid only
for image-invariant data such as distance checks, tag, and molecule, and must
not be used to read image-dependent properties.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c0df027-0ad6-4d68-ab48-9eba3bce18be
📒 Files selected for processing (3)
source/lmp/pair_deepmd.cppsource/lmp/pair_deepmd.hsource/lmp/tests/test_lammps_compact.py
🚧 Files skipped from review as they are similar to previous changes (1)
- source/lmp/pair_deepmd.h
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
source/lmp/tests/test_lammps_compact.py:129
- The MPI test helper only looks for an executable named
lmp. In many build/install setups (including locally built LAMMPS) the binary may have a different name/path, which would silently skip all MPI coverage for compact selection. Consider allowing an env override and/or trying common candidate names before skipping.
mpirun = shutil.which("mpirun")
lmp = shutil.which("lmp")
if mpirun is None or lmp is None:
pytest.skip("MPI compact tests require mpirun and the lmp executable")
Coding-Agent: Codex\nCodex-Version: codex-cli 0.144.6\nModel: gpt-5.6-sol\nReasoning-Effort: xhigh
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
source/lmp/pair_deepmd.cpp:1041
- The compact-selection settings reset clears compact_center_tags_ and compact_selected_, but leaves compact_is_center_ intact. If the pair style is reconfigured in the same LAMMPS session (or if neighbor->ago is nonzero on the first compute), apply_compact_selection() can reuse stale center-membership flags because the recompute condition doesn’t consider compact_center_tags_ being refreshed. Clearing compact_is_center_ alongside the other compact caches avoids incorrect center detection.
compact_center_group_id_.clear();
compact_center_tags_.clear();
compact_selected_.clear();
Cache a single-rank compact atom map and filtered neighbor list between LAMMPS rebuilds, update only selected coordinates on ordinary steps, and scatter forces directly unless model-deviation communication requires full arrays. Unsupported MPI and atomic-parameter configurations continue to use the generic backend selection path. Add regression coverage comparing the packed path with the generic type-minus-one fallback. Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
source/lmp/pair_deepmd.cpp:1253
- compact_is_center_ is not reset in settings(). If pair_style deepmd is reconfigured within the same LAMMPS session, apply_compact_selection() may reuse stale cached center membership (since it only rebuilds compact_is_center_ on size mismatch/neighbor rebuild), leading to incorrect compact selection.
compact_center_group_id_.clear();
compact_center_tags_.clear();
compact_selected_.clear();
source/lmp/pair_deepmd_kokkos.cpp:183
- deepmd/kk checks compact_selection_enabled() after calling PairDeepMD::init_style(), which can trigger compact-selection initialization/error paths and may prevent the intended "unsupported" diagnostic from being shown. Check and error out before calling PairDeepMD::init_style() so users reliably get the deepmd/kk-specific message.
// Base setup and the full neighbor-list request.
PairDeepMD::init_style();
if (compact_selection_enabled()) {
error->all(FLERR,
"pair style deepmd/kk does not yet support center_group compact "
"evaluation; use pair style deepmd");
}
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.
Summary
center_group,environment_cutoff, andinclude_moleculeoptions topair_style deepmd.-1, reusing the existing DeepPot compaction, neighbor remapping, communication, and output-scattering path.pair_style deepmd/kkuntil accelerator parity is implemented.Validation
cmake --build source/build --target deepmd_lmp -j2pytest source/lmp/tests/test_lammps_compact.py -v— 7 passedpytest source/lmp/tests/test_lammps_option_parsers.py -v— 25 passedruff check .ruff format .clang-format --dry-run --Werror source/lmp/pair_deepmd.cpp source/lmp/pair_deepmd.h source/lmp/pair_deepmd_kokkos.cppThe compact tests cover orthogonal and restricted triclinic boxes, periodic boundary selection, complete-molecule promotion, zero outputs for excluded atoms, selected-only model deviation, cache invalidation when selection changes, cross-domain MPI selection, and ranks with no selected owned atoms.
Addresses #5942.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
Summary by CodeRabbit
New Features
pair_style deepmd.Bug Fixes
pair_style deepmd/kk.Documentation