Repository navigation
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing. Review the Copilot code review billing settings.
for more information, see https://pre-commit.ci
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
njzjz-bot
left a comment
There was a problem hiding this comment.
Agent: dot
Reviewed the four-file change at exact head 16409639b843f10491dcea9946e9c7a43bf4c009 against 4f78c2d0. No actionable production defect found in this bounded trace-sample change. The isolated center is an owned node even in the four-atom fixture; the connected cluster contains the highest valid type, while sample size, edge-count derivation, graph ABI and dense compatibility metadata remain unchanged. The added export and checkpoint tests check relevant behavior rather than introducing a second neighbor contract.
Independent verification: executed the exact NumPy geometry/type-construction statements for 288 combinations of frame count, node count, type count, cutoff and float precision. Periodic minimum-image checks confirm isolation, at least two incoming/outgoing neighbors for the highest-type node, valid type bounds and deterministic construction. The original modulo-only assignment fails the 118-type boundary control. All four changed Python files compile syntactically.
These checks do not execute Torch graph construction, export, eager/export energy-force-virial parity or native/CUDA integration: PyTorch is unavailable in this executor. The author's reported full CPU results are not independently reproduced here. Current-head Python and C++ CI are still running. The green Test CUDA workflow has both Python/C++ CUDA jobs skipped, so it is not GPU validation. No code, branch, workflow or merge changes were made.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6090 +/- ##
==========================================
- Coverage 78.08% 77.83% -0.25%
==========================================
Files 1171 1171
Lines 141657 141661 +4
Branches 5072 5078 +6
==========================================
- Hits 110615 110264 -351
- Misses 29102 29460 +358
+ Partials 1940 1937 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
wanghan-iapcm
left a comment
There was a problem hiding this comment.
LGTM as a hardening of the graph trace sample coverage. Note that the end-to-end export parity test also passes on the pre-fix code, so the new structural tests pin the sample layout rather than reproduce a user-visible failure.
Summary
Replace #6068 with a deliberately smaller change based on current master (
4f78c2d0).Current master already shares bounded, sel-independent graph trace inputs between compilation and export. The remaining gap is sample coverage: a seven-atom sample assigned types modulo the type-map length never exercises type 117 in a 118-type model, and random geometry does not guarantee an isolated owned center.
No extra neighbor-contract abstraction, hot-path dispatch, graph ABI, C++ source, or metadata schema changes are introduced. Legacy
sel/nneiremain available to dense consumers and existing C++ readers. DPA1 auto-sel and normalization/statistics are deliberately unchanged; removing their semantic dependence on sel belongs to #5824. This PR does not claim to complete all of #5825.Validation
CPU, using the installed native extension:
ruff check .,ruff format ., andgit diff --check.GPU execution and a fresh C++ rebuild were not performed. Metadata compatibility is asserted, not presented as an executed C++ integration test.
Refs #5825, #5824, #5755. Supersedes #6068.
Summary by CodeRabbit