fix(dpmodel): mask virtual EnvMat centers - #5833
Conversation
Use safe type indices and neutral normalization values before zeroing virtual-center descriptors. Add radial, angular, and strict Array API regression coverage. Coding-Agent: Codex Codex-Version: codex-cli 0.144.4 Model: gpt-5.6-sol Reasoning-Effort: xhigh
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesEnvMat virtual center handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
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 #5833 +/- ##
==========================================
+ Coverage 79.03% 79.24% +0.20%
==========================================
Files 1055 1073 +18
Lines 122233 125306 +3073
Branches 4401 4569 +168
==========================================
+ Hits 96607 99293 +2686
- Misses 24061 24376 +315
- Partials 1565 1637 +72 ☔ 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
There was a problem hiding this comment.
The dpmodel fix is correct and array-API-clean: std -> 1 is applied before the divide (NaN prevented, not just masked), the reshape broadcasts correctly for both radial and full modes, and it's a provable no-op for all-real centers. jax and pt_expt inherit it. Two inline notes; one cross-backend parity note here:
pt and pd carry the same unmasked-center pattern (parity / defense-in-depth, likely a follow-up). deepmd/pt/model/descriptor/env_mat.py (t_avg = mean[atype]; t_std = stddev[atype]) and deepmd/pd/model/descriptor/env_mat.py do the identical unguarded mean[atype] this PR fixes for dpmodel, and aren't touched here. In practice it's not an end-to-end bug — base_atomic_model.forward_common_atomic pre-clamps atype<0 to 0 before the descriptor runs (and zeroes outputs after), so the standard model path never feeds a negative center to mean[-1]; the dpmodel fix (and these tests) are really hardening the direct-call path. So this is a low-priority backend-parity cleanup rather than a correctness fix, but worth tracking so pt/pd don't diverge from dpmodel/the C++ CPU reference.
Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
…virtual-center-5628
_make_env_mat already zeroes em, diff and sw wherever nlist < 0, so a virtual center -- whose neighbor row is empty by the neighbor-list contract -- only leaves this function nonzero because normalization shifts it: xp.take with the negative sentinel silently selects the last real type's davg/dstd. Gather with a safe index and neutralize the offset and scale instead. Masking em/diff/sw again afterwards makes the descriptor depend on atype_ext, which the compiled pt_expt DPA2 lower turns into wrong forces (TestCompiledVaryingNatoms, 100% of force elements). A scalar no-op multiply in the same place is fine, so it is the atype dependency, not the extra op.
|
Rebased onto master and narrowed the fix in What was failing.
So it is not the semantics and not the What the fix is now. Tests. |
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Both points are resolved, and I want to note that the second one landed somewhere better than what I proposed.
I had suggested either extending the center mask to diff/sw or softening the comment. 6d67a1257 did the former, and 27f2a5a0f then reverted it in favour of neutralizing the normalization instead -- zero offset and unit scale for virtual centers. That is the right call given what the new comment discloses, and the comment now grounds the guarantee in the neighbor-list contract rather than claiming protection against arbitrary malformed lists, which is what I was actually objecting to. The tests were updated to match rather than left behind: test_mixed_centers_keep_virtual_rows_at_zero uses a contract-honoring empty neighbor row and says so, instead of the malformed-list scenario an earlier round advertised. Code and tests agreeing on the narrower claim is worth more than the broader claim was.
The strict Array API gap is closed too -- the mixed case proves the real row stays nonzero while the virtual row stays at zero, so it now discriminates in both directions rather than only checking that nothing crashes. Using dstd = zeros in the all-virtual case is a nice touch: it fails loudly if masking ever moves after the division.
I ran these against a master baseline instead of reading them. Both new cases fail unpatched -- the all-virtual one with RuntimeWarning: divide by zero encountered in divide, which is exactly the hidden NaN the fix is preventing -- and all five pass after. On why nothing caught this before: xp.take(davg, -1) on NumPy silently selects the last real type's row, so a virtual center was getting a plausible-looking normalization rather than an error, which is invisible without an explicit assertion that virtual rows stay at zero.
One request, and it is about something the comment reveals rather than about this diff.
Approving.
The zero-output guarantee for virtual centers rests on the neighbor row being empty. Document that the in-tree neighbor-list builder fills a virtual atom's row with -1 by construction. Coding-Agent: opencode opencode-Version: 1.18.9 Model: ustc/deepseek-v4-flash Reasoning-Effort: max
Summary
Why existing tests missed this
The direct EnvMat unit test only used real center atoms. A separate model-level virtual-atom test did not expose the bug because the model masks virtual outputs after fitting and initializes descriptor averages to zero, so the invalid negative normalization lookup was not observable. The new tests use nonzero averages and a zero placeholder scale, which make both the negative-index behavior and masking order observable. The broader compiled DPA2 varying-natoms test had previously never been connected to virtual-center normalization; it is retained as a cross-cutting compile/autograd guard and was also reproduced after an isolated CI mismatch.
Validation
TestCompiledVaryingNatoms::test_compiled_matches_uncompiled_varying_natoms_dpa2: passed locally in 205.82 s, covering four compiled/eager training steps, changing frame/atom counts, force and virial outputs, and second-order force-loss gradientsCloses #5628
Coding agent: Codex
Codex version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning effort: xhigh
Summary by CodeRabbit
Bug Fixes
Tests