fix(pt,dpmodel): drop out-of-rcut neighbors in _format_nlist pad branch#5529
Conversation
forward_lower's _format_nlist only filtered neighbors beyond rcut in its sort branch (n_nnei > nnei or extra_nlist_sort). When the input neighbor list width is <= nnei (sum of sel) and extra_nlist_sort is False, the pad branch returned the nlist unchanged. The C++/LAMMPS path builds the neighbor list with rcut+skin and does NOT rcut-filter before forward_lower, so on sparse systems (per-atom neighbor count <= nnei, e.g. discussion deepmodeling#5438: width 39 < 100) out-of-rcut neighbors leaked into the descriptor. Because the pad branch does not re-sort, the leaked contribution is order-dependent: reversing the nlist changes the energy by ~1e-4 (se_r) / ~4e-6 (se_a). Fix: the pad branch now also drops neighbors with rr > rcut (no re-sort needed, these descriptors reduce over neighbors order-independently). Applied to both the pt and dpmodel backends (dpmodel is shared by pt_expt). The exported pt_expt graph is unaffected: export forces extra_nlist_sort=True, so it takes the sort branch. Regression test test_format_nlist_overcut.py: over-cut (rcut+2) forward_lower, as-is and reversed, must match the rcut-bounded reference for se_a and se_r. The reversed cases fail without this fix.
📝 WalkthroughWalkthroughThe PR modifies neighbor-list formatting in both dpmodel and pt model implementations to filter out neighbors with distances exceeding the cutoff radius in the non-sorted path, and adds regression tests validating this behavior across descriptor flavors and neighbor orderings. ChangesNeighbor-list cutoff filtering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5529 +/- ##
=======================================
Coverage 82.19% 82.19%
=======================================
Files 891 891
Lines 101599 101612 +13
Branches 4242 4242
=======================================
+ Hits 83507 83521 +14
Misses 16789 16789
+ Partials 1303 1302 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The pt-side test_format_nlist_overcut.py only exercises deepmd/pt. Add the dpmodel (numpy) counterpart so the deepmd/dpmodel _format_nlist change (shared by pt_expt) is covered: over-cut call_lower, as-is and reversed, must match the rcut-bounded reference (reduced + per-atom energy). The reversed case fails with the dpmodel fix reverted.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
source/tests/common/dpmodel/test_format_nlist_overcut.py (1)
66-73: ⚡ Quick winConsider adding test coverage for se_a descriptor.
The test currently only covers the
se_e2_r(se_r) descriptor. According to the PR objectives, the fix applies to both se_a and se_r descriptors, and the pt backend test covers both. Consider adding a parameterized test or a second test case for se_a to ensure comprehensive coverage of the dpmodel backend as well.🤖 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/tests/common/dpmodel/test_format_nlist_overcut.py` around lines 66 - 73, The test method test_overcut_matches_canonical currently only covers the se_e2_r (se_r) descriptor, but the fix applies to both se_a and se_r descriptors according to the PR objectives. Add test coverage for the se_a descriptor by parameterizing the test method to cover both descriptor types (se_a and se_r) or by adding a separate test case that validates the se_a descriptor with the same overcut matching logic to ensure comprehensive coverage of the dpmodel backend.
🤖 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.
Nitpick comments:
In `@source/tests/common/dpmodel/test_format_nlist_overcut.py`:
- Around line 66-73: The test method test_overcut_matches_canonical currently
only covers the se_e2_r (se_r) descriptor, but the fix applies to both se_a and
se_r descriptors according to the PR objectives. Add test coverage for the se_a
descriptor by parameterizing the test method to cover both descriptor types
(se_a and se_r) or by adding a separate test case that validates the se_a
descriptor with the same overcut matching logic to ensure comprehensive coverage
of the dpmodel backend.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d87efcf2-4707-4808-838b-ba0f1583a1fc
📒 Files selected for processing (1)
source/tests/common/dpmodel/test_format_nlist_overcut.py
Summary
forward_lower's_format_nlistonly filtered out-of-rcutneighbors in its sort branch (n_nnei > nnei or extra_nlist_sort). When the input neighbor-list width is<= nnei(sum(sel)) andextra_nlist_sortisFalse, it took the pad branch and returned the nlist unchanged — never dropping neighbors beyondrcut.The C++/LAMMPS path (
DeepPotPT.cc→copy_from_nlist→padding) builds the neighbor list withrcut + skinand does not rcut-filter beforeforward_lower(the in-code comment incommonPT.his explicit: "No truncation or distance sorting is done — the model's format_nlist handles that"). Its width is the per-atom neighbor count, which on sparse systems is<= nnei— exactly the case in discussion #5438 (width 39 < 100). In that regime, out-of-rcutneighbors leak into the descriptor. Because the pad branch does not re-sort, the leaked contribution is order-dependent: reversing the nlist changes the energy by ~1e-4(se_r) / ~4e-6(se_a).This is the same root condition as #5438; that PR (#5524) closed it only for compressed se_a (by forcing
extra_nlist_sort). The uncompressed paths and se_r/se_t remained exposed.Fix
The pad branch now also drops neighbors with
rr > rcut(no re-sort — these descriptors reduce over neighbors order-independently). Applied to both theptanddpmodelbackends (dpmodelis shared bypt_expt).The exported
pt_exptgraph is unaffected: export forcesextra_nlist_sort=True, so it always takes the sort branch; the new pad-branch code is eager-only.Verification
test_format_nlist_overcut.py: over-cut (rcut+2)forward_lower, as-is and reversed, must match thercut-bounded reference for se_a and se_r (energy + force,1e-10). The reversed cases fail without this fix and pass with it.rel = 0.0, se_rrel = 4.4e-16(were4.3e-6/1.4e-4).test_jit(all models),test_forward_lower,test_permutation,pt_exptdescriptor + ener-model export, universal dp/pt model consistency.Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Tests