fix(dpmodel): preserve nopbc batch semantics - #6003
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. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthrough
ChangesNon-periodic batch normalization
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to This change converts legacy non-periodic zero-box placeholders to a non-periodic model input while preserving periodic boxes. The stated regression coverage includes boundary cases and NPY data-system integration, with no current merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
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.
Describe the default_mesh conversion in terms of the canonical model input rather than implying that normalize_batch inspects box values.
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
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. 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. 🚀 New features to boost your workflow:
|
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
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.
Merge upstream master at 28b7d06 so fresh PR checks include the corrected training-argument fixture for EMA checkpoint retention. The nopbc changes remain limited to batch normalization and its regression tests. Validation: ruff format .; ruff check .; git diff --check; focused pytest coverage for training arguments, nopbc batches, and charge states (13 passed, 10 subtests passed). Coding-Agent: Codex Codex-Version: codex-cli 0.153.4 Model: gpt-6-astra Reasoning-Effort: xhigh
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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