-
Notifications
You must be signed in to change notification settings - Fork 648
Conversation
Convert non-periodic default_mesh encodings to box=None before normalize_batch drops the legacy metadata. This prevents pt_expt neighbor construction from treating zero box placeholders as periodic cells.\n\nAdd regression coverage for standard and mixed-type mesh encodings, periodic box preservation, and a real nopbc NPY data system.
|
No actionable comments were generated in the recent review. 🎉 i️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthrough
ChangesNon-periodic batch normalization
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change converts legacy non-periodic batch placeholders to the canonical non-periodic representation while preserving periodic batches. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The implementation translates non-periodic default_mesh encodings to box=None before metadata removal. The tests cover non-periodic and periodic batches, missing metadata, and a temporary nopbc system, satisfying issue
✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes loss of non-periodic (nopbc) semantics during DeepmdDataSystem batch normalization in deepmd.dpmodel.utils.batch.normalize_batch(). It translates the legacy default_mesh encoding for non-periodic systems (mesh sizes 0/1) into the canonical model input form (box=None), preventing downstream periodic-only code paths from attempting to invert the all-zero placeholder cell.
Changes:
- Convert non-periodic
default_meshencodings (size 0 or 1) tobox=Noneduringnormalize_batch()(beforedefault_meshis dropped). - Preserve periodic boxes unchanged for both standard-type and mixed-type batches.
- Add regression tests covering (a) the mesh-size boundary behavior and (b) a real temporary
nopbcNPY system with a zero-box placeholder.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
deepmd/dpmodel/utils/batch.py |
Implements non-periodic semantic conversion (default_mesh size 0/1 ⇒ box=None) while keeping periodic boxes intact. |
source/tests/common/test_batch_nopbc.py |
Adds regression coverage for legacy mesh encodings and an end-to-end nopbc system batch ensuring inputs["box"] is None. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Describe the default_mesh conversion in terms of the canonical model input rather than implying that normalize_batch inspects box values.
@njzjz-bot
njzjz-bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed the non-periodic batch-normalization path and its coverage. The legacy mesh-size 0/1 encodings are translated before metadata is dropped, periodic encodings remain intact, and the input batch is not mutated. I found no blocking issues.
Coding agent: Codex
Codex version: codex-cli 0.151.0
Model: gpt-5.6-sol
Reasoning effort: xhigh
Codecov Report
✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.68%. Comparing base (8cfd46e) to head (4399883).
Additional details and impacted files
@@ Coverage Diff @@ ## master #6003 +/- ## ========================================== - Coverage 79.10% 77.68% -1.42% ========================================== Files 1105 1105 Lines 130981 130983 +2 Branches 4771 4765 -6 ========================================== - Hits 103609 101758 -1851 - Misses 25686 27546 +1860 + Partials 1686 1679 -7
☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.
🚀 New features to boost your workflow:
- ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
@wanghan-iapcm
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diagnosis is right and the fix is correct. The layer is right too, which is worth recording: doing this at the producer instead — if not self.pbc: b_data["box"] = None in DeepmdDataSystem.get_batch_standard/get_batch_mixed — would break TensorFlow and JAX training on nopbc systems, since get_batch() is shared with backends that require box to be an array. normalize_batch is the first point downstream of that contract.
Two notes, neither blocking.
Uh oh!
There was an error while loading. Please reload this page.
Summary
default_meshencodings (sizes 0 and 1) to the canonical model inputbox=NonenopbcNPY systemRoot cause
DeepmdDataSystemrepresents a valid non-periodic system with an empty or single-elementdefault_meshand an all-zeroboxplaceholder.normalize_batch()dropped the mesh metadata but retained the placeholder, so downstream code treated the non-Nonebox as periodic and attempted to invert a singular cell.The fix performs the semantic conversion before the metadata is discarded, without mutating the input batch.
Fixes #6002.
Testing
.venv/bin/python -m pytest source/tests/common/test_batch_nopbc.py source/tests/common/test_batch_charge_state.py -q(10 passed, 10 subtests passed).venv/bin/ruff check ..venv/bin/ruff format --check .git diff --checkSummary by CodeRabbit
Bug Fixes
Tests