fix(dpmodel): preserve padding when forced-sorting short neighbor lists#5764
fix(dpmodel): preserve padding when forced-sorting short neighbor lists#5764njzjz-bot wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughNeighbor-list utilities now support backend-specific sizing and ranges, validate static shapes, and normalize neighbor lists to fixed widths while preserving padding during optional distance sorting. Tests cover partially padded and fully padded forced-sort inputs. ChangesNeighbor-list utilities
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
njzjz-bot
left a comment
There was a problem hiding this comment.
The core fix is correct: using the padded ret throughout the forced-sort branch preserves the requested width and matches the corresponding PyTorch behavior. However, this PR is currently not mergeable: its parent is dd428dbb, it is 124 commits behind the current base, GitHub reports CONFLICTING, and pre-commit.ci stopped at the mergeability check. The stale base also makes the diff include many already-merged nlist.py changes. Please rebase onto current master, resolve the conflict, and retain the minimal three-source-line change plus the regression test before merging.
As a non-blocking test improvement after rebasing, an exact array assertion would verify that every remaining slot is -1; the current set-based check would also accept duplicate real neighbors.
Coding agent: Codex
Codex version: codex-cli 0.144.0-alpha.4
Model: gpt-5.6-sol
Reasoning effort: xhigh
078b9b5 to
1a484b0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5764 +/- ##
==========================================
- Coverage 79.65% 79.52% -0.13%
==========================================
Files 1015 1015
Lines 115781 115781
Branches 4272 4272
==========================================
- Hits 92229 92079 -150
- Misses 22008 22157 +149
- Partials 1544 1545 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| ``nlist`` instead of the padded ``ret``, so the output had the original | ||
| short width instead of ``nnei`` and the ``-1`` padding was lost. | ||
| """ | ||
|
|
There was a problem hiding this comment.
Non-blocking (test organization): consider folding these cases into the existing source/tests/common/dpmodel/test_nlist.py::TestNList, which already targets format_nlist with test_nlist_eq / test_nlist_st / test_nlist_lt and a shared fixture (self.md, self.coord_ext, self.atype_ext, self.expected_nlist), rather than a separate file that rebuilds its own nlist.
Root cause of why the existing UTs missed this: test_nlist_st already covers the short list (n_nnei < nnei), but every existing test calls format_nlist with the default extra_nlist_sort=False, so the buggy if extra_nlist_sort or n_nnei > nnei: branch was never crossed with a short input (test_nlist_lt hits the sort branch but with ret == nlist, so the padding bug is invisible). The untested cell is the intersection extra_nlist_sort=True × n_nnei < nnei. Adding extra_nlist_sort=True variants (e.g. test_nlist_st_sort) to the {eq, st, lt} set would make that matrix explicit and keep all format_nlist coverage in one place.
Fold the forced-sort regression coverage into the existing neighbor-list test matrix, including the all-padding edge case. Coding-Agent: Codex Codex-Version: codex-cli 0.144.1 Model: gpt-5.6-sol Reasoning-Effort: xhigh
eb52ebb to
8b30d37
Compare
Fix #5629
Problem
format_nlistpads the input nlist to widthnneiwhen the input is shorter, but the forced-sort branch (extra_nlist_sort=True) then re-read the shape from the original unpaddednlistinstead of the paddedret. This caused:n_nnei, notnnei)ret[..., :nnei]slice to return a tensor with the original short width-1padding to be lostassert ret.shape[-1] == nneicheck to fail for static shapesThis is especially reachable for linear atomic models, which always request sorted lower neighbor lists via
need_sorted_nlist_for_lower().Fix
Use the padded
rettensor (instead of the originalnlist) throughout the forced-sort branch. Added explanatory comments.Test
Added
source/tests/common/dpmodel/test_format_nlist_short_padding.pywith three test cases:nnei=4, verifies output width is 4 and real neighbors are preserved.-1with widthnnei.Attribution
Generated with opencode using model
glm-5.2.Recommended reviewers
@njzjz (maintainer of the dpmodel code)
Summary by CodeRabbit
Improvements
Tests