docs+tests(bias): pin set/change-by-statistic semantics for bridged models - #5966
Conversation
Close deepmodeling#5927 as designed. The model energy decomposes as E = E_model + E_bias; set-by-statistic DEFINES E_bias as the per-type statistic of the raw labels (or user values), independent of E_model -- it ignores a trained network and it equally ignores the analytical ZBL term of a bridged model. change-by-statistic fits the residual against the complete model prediction (bridged predictor since deepmodeling#5910). The 'double count' described in the issue is therefore not a bridging bug but the uniform, defined behavior of the set mode for any model with nonzero E_model; no code change is made. - doc/model/change-bias.md: precise definitions of the two modes and the guidance that a self-consistent calibration of a bridged model needs change-by-statistic. - doc/model/dpa4.md: note in the ZBL section. - Semantic pin tests in pt (SeZMModel) and dpmodel (LinearEnergyAtomicModel composition): set-by-statistic equals the raw-label least-squares fit exactly -- guarding against a future 'fix' that would subtract the analytical term and silently create a third, model-dependent mode. Verified conformance of the linear_ener composition path in both backends: children compute no output statistics; the composition-level set fits raw labels.
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe changes document ChangesOutput-bias calibration
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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 |
![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 `@doc/model/change-bias.md`:
- Around line 24-26: Update the prediction-offset description in the
set-by-statistic discussion to state that set leaves the configuration-dependent
E_model uncompensated, along with any residual from the raw-label least-squares
fit; remove the claim that each calibration prediction is offset by the data
mean of E_model.
🪄 Autofix
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: d255cd38-cff2-45c8-9b21-b47dd30d7b25
📒 Files selected for processing (4)
doc/model/change-bias.mddoc/model/dpa4.mdsource/tests/common/dpmodel/test_zbl_bridging.pysource/tests/pt/model/test_sezm_model.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5966 +/- ##
==========================================
- Coverage 79.60% 79.36% -0.25%
==========================================
Files 1085 1085
Lines 126405 126405
Branches 4598 4598
==========================================
- Hits 100631 100319 -312
- Misses 24120 24434 +314
+ Partials 1654 1652 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
Three independent review passes converged on the two inline findings below. I also verified the existing CodeRabbit comment on change-bias.md lines 24–26 and did not duplicate it.
Validation:
- Both newly added targeted tests pass locally.
- A SeZM+ZBL self-label reproduction shows
set-by-statisticchanges the predictions to exactly 2× their original values with a full-rank type-count matrix (maximum error2.84e-14). - All reported PR checks currently pass.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
njzjz
left a comment
njzjz
left a comment
There was a problem hiding this comment.
@njzjz-bot has commented above.
- change-bias.md: drop the incorrect 'offset by the data mean of E_model' claim (the remaining error is the configuration-dependent E_model plus the raw-label least-squares residual) and the 'double-counts nothing' claim (the raw-label fit can absorb the composition-correlated component of E_model into E_bias, which the forward pass then adds again). - pt + dpmodel set-by-statistic tests: seed a nonzero out_bias before the call (an accidental additive implementation would shift the result) and repeat the call to pin idempotence; mirrored in dpmodel because its bias storage is separate.
There was a problem hiding this comment.
Three independent review passes covered functional correctness, test/regression behavior, and API/compatibility/maintainability on the current head a6a2bf690e693144ea6f478f3037d4ef4b574e29.
No new actionable findings remain on the current diff, so there are no new inline comments to attach. The earlier line-specific findings about the set semantics and the missing nonzero-bias/idempotence coverage are addressed by the latest commit.
Validation:
source/tests/common/dpmodel/test_zbl_bridging.py::test_set_by_statistic_fits_raw_labels_by_definition: passed.source/tests/pt/model/test_sezm_model.py::TestSeZMModelBridging::test_set_by_statistic_fits_raw_labels_by_definition: passed.- Documentation and Python CI checks pass.
- The remaining failed C++ job stopped while downloading the Paddle inference archive with
Transferred a partial file; it did not reach code compilation and is unrelated to this PR.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
fec4497
3 participants
Close #5927 — resolved as designed, with documentation and semantic pin tests instead of a behavior change.
The definitions (verified against the code)
The model energy decomposes as
E = E_model + E_bias, whereE_modelis everything the model computes (learned network, analytical ZBL term, or alinear_enercombination) andE_biasis the per-type output bias.set-by-statisticassignsE_biasdirectly: user values, or the per-type least-squares statistic of the raw labels. It never calls a model forward (compute_output_statswithmodel_forward=None, stored withadd=False) — it is independent ofE_modelby definition, ignoring a trained network and the analytical ZBL term alike.change-by-statisticfits the residual of the labels against the complete model prediction (the bridged predictor since fix(pt): calibrate DPA4 bias and support PyTorch 2.13 compile #5910) and adds the delta to the existing bias.Under these definitions the "double count" described in the issue is not a bridging bug:
setuniformly ignores all ofE_modelfor every model kind. A bridged model aftersetcarries no compensation for the mean ZBL contribution — exactly as a trained plain model aftersetcarries no compensation for its network output. The two modes "disagree" because they are defined to answer different questions; a self-consistent calibration ischange's job.Verified conformance of the
linear_enercomposition path in both backends: children compute no output statistics (compute_or_load_out_stat=False); the composition level performs onesetfit on the raw labels. Also verified: the ZBL term contributes exactly zero for isolated atoms, so its own statistics are triviallybias = 0.Changes
doc/model/change-bias.md: precise definitions of the two modes, and the guidance that calibrating a bridged (or any nonzero-E_model) model self-consistently requireschange-by-statistic.doc/model/dpa4.md: note in the ZBL section.SeZMModel+ dpmodelLinearEnergyAtomicModelcomposition):set-by-statisticequals the raw-label least-squares fit exactly — guarding against a future "fix" that would subtract the analytical term and silently create a third, model-dependent mode.Known limitations
model.change_out_bias(thedp change-bias --mode setand finetune routes) and the dpmodel composition out-stat; the pt training-init chain was verified in-session and funnels into the same pinned branch.InnerPotentialadds the full ZBL over the whole cutoff (not only belowbridging_r_outer), so the label-side ZBL at equilibrium geometries is small but not strictly zero; the docs state the offset plainly.Summary by CodeRabbit
Documentation
setandchangestatistic modes handle model energy, analytical contributions, labels, residuals, and existing bias.Tests
set-by-statisticcalibration uses raw energy labels independently of learned and ZBL model contributions.