test(pt_expt): document compiled neighbor type equivalence - #5860
Conversation
Clarify that compiled training intentionally passes merged global candidates because the lower model performs the same non-mixed type layout. Add an adversarial A/A/B regression proving early and lower-layer type splits produce the same final neighbor list under the established contract. Coding-Agent: Codex Codex-Version: codex-cli 0.144.4 Model: gpt-5.6-sol Reasoning-Effort: xhigh
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change documents the dense compiled-training neighbor-list contract and adds regression coverage for deferred type splitting versus direct type-aware neighbor-list construction. ChangesNeighbor-list layout
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5860 +/- ##
==========================================
+ Coverage 78.58% 79.22% +0.63%
==========================================
Files 1050 1072 +22
Lines 120637 125041 +4404
Branches 4356 4541 +185
==========================================
+ Hits 94801 99060 +4259
- Misses 24278 24357 +79
- Partials 1558 1624 +66 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Possible reviewers based on changed lines, exact file history, and exact-file review history:
No review request was made automatically. Coding agent: Codex |
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
The false-positive verdict is correct, and I checked it rather than taking it on trust. build_neighbor_list sorts by distance, drops self, truncates globally to sum(sel), masks beyond rcut, and only then applies nlist_distinguish_types as the final step - so the flag toggles nothing about which atoms survive. I reproduced the test's scenario directly (merged = [1, 2], both orderings give [1, -1]), and a randomised sweep over roughly 400 configurations - 1-3 types, varying sel, nloc 1-5, 0-7 ghosts, tied coordinates, virtual atoms, exclude_types, extra_nlist_sort on and off, multiple frames - produced no mismatches. I also confirmed the compiled path cannot bypass format_nlist: the graph route is gated on mixed_types(), and spin models are rejected from compiled training outright.
Documenting this at the call site is worth doing, and the conclusion matches what I concluded on the same report in #5491.
One comment inline, on the fixture. Non-blocking.
Smaller notes:
-
The test calls
nlist_distinguish_typesdirectly and never goes throughformat_nlist, which is the seam the new comment is actually about - specifically the equal-width branch preserving order, the split being applied unconditionally, and theextra_nlist_sort=Truepath used by compressed models. Assertingformat_nlist(build(distinguish_types=False))againstformat_nlist(build(distinguish_types=True))would pin the claim where it is load-bearing rather than one level below it. -
The comment says "forward_common_lower always calls model.format_nlist", but what
_trace_and_compiletraces ismodel.forward_lower, and each concrete model'sforward_lowercallscall_common_lowerdirectly.forward_common_loweronly appears in the spin plumbing, so it is not in this path at all. The substantive claim survives -call_common_lowerdoes callformat_nlistunconditionally - but someone grepping for the name the comment gives them will not find it here. -
Worth one more sentence on pair exclusion, since the ordering differs between the two paths: in the eager build, exclusion is applied after the type split; here it is applied before, with the split deferred to
format_nlist. That would normally let a farther same-type neighbour slide into a freed slot. It does not, but only becausePairExcludeMaskis keyed on the type pair, so every neighbour of an excluded type is erased uniformly and the bucket ends up empty either way. That is not obvious from the current text.
Separately, and not a request on this PR: the fixture makes the underlying design choice very visible - a type-1 atom at distance 2.0 is inside rcut=3.0 and still absent from the final list. That is the intended contract and the PR is right to say a per-type-safe candidate superset would be a separate cross-backend change. But it is worth having that written down somewhere more discoverable than a scan-triage thread, since this is the second time the same report has been raised.
Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/common/dpmodel/test_nlist.py`:
- Around line 231-240: Replace the expected-value construction around
build_neighbor_list with an independent type-aware reference that groups
candidates by atom type, sorts each group by distance, and truncates using the
corresponding sel limits. Compare both lower_formatted and early_formatted
against this independently computed reference, rather than comparing the two
implementation paths to each other.
🪄 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: e7f034d7-7208-4ec0-a25c-2fb69abf4edc
📒 Files selected for processing (1)
source/tests/common/dpmodel/test_nlist.py
Validate both neighbor-list formatting paths against a standalone per-type distance-sort and truncation reference. Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/common/dpmodel/test_nlist.py`:
- Around line 45-53: Update _reference_type_neighbor_list to build and
distance-sort one global candidate list, truncate it to sum(sel), then populate
each type bucket from those globally selected neighbors while preserving
per-type limits. Add a test case verifying that a farther type-specific neighbor
outside the global cap is excluded.
🪄 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: 47d61283-8f9d-47e6-98aa-a347292f0d6b
📒 Files selected for processing (1)
source/tests/common/dpmodel/test_nlist.py
Coding-Agent: Codex\nCodex-Version: codex-cli 0.144.6\nModel: gpt-5.6-sol\nReasoning-Effort: xhigh
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Coverage gap addressed, and thoroughly. Every case I named is now present and, importantly, in the right place: it is a method on TestDPModelFormatNlist beside the other short/equal/long neighbor-list cases rather than a separate class rebuilding its own fixture.
The one that mattered is same_type_nearest. With sel = [2, 1] and three type-0 neighbours at 1.0/1.5/2.5 against a type-1 at 2.0, nlist_distinguish_types finally has to choose the nearest among several same-type candidates, and the 2.5 atom is correctly displaced by the global cap. That is the mechanism the whole equivalence claim rests on and the one thing the old single-slot fixture could not exercise. batched_ghost_ties_virtual_padding covers the rest in one case - two frames, nloc=2, nall > nloc, three atoms at an exact distance tie, a virtual atype < 0, and a padded bucket with an explicit assertion that padding is present. global_cap_excludes_far_type pins the disputed case from the report itself.
You also went past what I asked in the way that counts most. _reference_type_neighbor_list is an independent brute-force oracle, and both the lower split and the early split are asserted against it rather than merely against each other. The original test's weakness was that comparing the two paths to one another cannot detect a shared misconception; comparing both to an independent construction can.
One note purely for the record, requiring no change: I went looking at whether the exact-tie case depends on unspecified ordering, since build_neighbor_list calls xp.argsort(rr, axis=-1) without stable=True while the same function passes it explicitly a few lines later. It does not - the array API specifies stable=True as the default for argsort, so the compat namespace guarantees it, and I confirmed numpy, torch and jax all produce identical orderings on a tie-heavy array. The tie ordering the fixture relies on is guaranteed rather than incidental.
The training.py comment accurately describes what the test now demonstrates, and the change remains behavior-preserving.
109ae09
3 participants
Closes #5670 as a false positive.
Summary
Why the reported divergence does not occur
This matches the maintainer conclusion on the same report in PR #5491:
#5491 (comment)
If the desired behavior is instead to retain a per-type-safe candidate superset before truncation, that is a separate cross-backend neighbor-list contract change affecting eager, inference, export, and compiled paths.
Why existing tests did not make this obvious
Validation
The local pt_expt training module could not be collected because the installed libdeepmd_op_pt.so has a stale PyTorch ABI; this PR changes no executable behavior, and the backend-independent contract regression passes.
Coding agent: Codex
Codex version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning effort: xhigh
Summary by CodeRabbit
Documentation
Tests