fix(tf): honor change-bias --numb-batch#5703
Conversation
The TensorFlow data-based change-bias path dropped numb_batch: it called _apply_data_based_bias without it and then hardcoded ntest=1 in the change_energy_bias_lower call, so the bias was always estimated from a single frame per system regardless of -n/--numb-batch. Thread numb_batch into _apply_data_based_bias and forward it as ntest. Treat ntest <= 0 as "use all frames per system" in change_energy_bias_lower, matching the documented "0 means all data" behavior (backward compatible: the default ntest is 10 and no caller passed a non-positive value before). Add a test that a mock evaluator sees one frame for ntest=1 and all frames for ntest=0. Fix deepmodeling#5684
📝 WalkthroughWalkthroughThe TensorFlow change-bias entrypoint now forwards the Changesnumb_batch threading fix
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant change_bias
participant _apply_data_based_bias
participant change_energy_bias_lower
User->>change_bias: run change-bias with --numb-batch
change_bias->>_apply_data_based_bias: numb_batch
_apply_data_based_bias->>change_energy_bias_lower: ntest=numb_batch
alt ntest <= 0
change_energy_bias_lower->>change_energy_bias_lower: use all nframes
else ntest > 0
change_energy_bias_lower->>change_energy_bias_lower: use min(nframes, ntest)
end
change_energy_bias_lower-->>_apply_data_based_bias: updated bias
Suggested labels: 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
deepmd/tf/fit/ener.py (1)
1028-1029: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLogic fix correct; docstring for
ntestshould mention the<=0semantics.The frame-selection fix is correct. Consider updating the
ntestdocstring (Lines 1028-1029) to state thatntest <= 0means "use all frames," matching the inline comment at Line 1043, since this is a behavior-changing public API contract that other callers may rely on.📝 Proposed docstring update
ntest : int The number of test samples in a system to change the energy bias. + ``ntest <= 0`` means using all frames in the system.Also applies to: 1043-1044
🤖 Prompt for 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. In `@deepmd/tf/fit/ener.py` around lines 1028 - 1029, Update the `ntest` documentation in `ener.py` so it explicitly states the `<= 0` behavior: when `ntest <= 0`, all frames are used instead of limiting the test set. Keep the docstring aligned with the logic in the `ntest` handling section around the `change energy bias` path so callers understand the public API contract.
🤖 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.
Nitpick comments:
In `@deepmd/tf/fit/ener.py`:
- Around line 1028-1029: Update the `ntest` documentation in `ener.py` so it
explicitly states the `<= 0` behavior: when `ntest <= 0`, all frames are used
instead of limiting the test set. Keep the docstring aligned with the logic in
the `ntest` handling section around the `change energy bias` path so callers
understand the public API contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f3f1d28f-d5c8-4575-b155-6da0dd243842
📒 Files selected for processing (3)
deepmd/tf/entrypoints/change_bias.pydeepmd/tf/fit/ener.pysource/tests/tf/test_change_energy_bias.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5703 +/- ##
==========================================
- Coverage 81.97% 81.70% -0.28%
==========================================
Files 959 966 +7
Lines 105748 106147 +399
Branches 4102 4141 +39
==========================================
+ Hits 86684 86724 +40
- Misses 17573 17904 +331
- Partials 1491 1519 +28 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
njzjz-bot
left a comment
There was a problem hiding this comment.
Looks good to me. The data-based change-bias path now forwards --numb-batch through to change_energy_bias_lower, and the ntest <= 0 handling matches the documented “use all data” behavior. The new regression test exercises both the one-frame cap and the all-frames case. CI is green.
Minor non-blocking follow-up: updating the ntest parameter docstring in change_energy_bias_lower to explicitly mention ntest <= 0 would make the API contract clearer.
Reviewed by OpenClaw 2026.6.8 (844f405)
Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.5)
Problem
change-bias --numb-batchis documented as the number of frames to use per data system, with0meaning all data. On the TensorFlow data-based path this value was ignored:_change_bias_checkpoint_filecalled_apply_data_based_bias(...)withoutnumb_batch, and_apply_data_based_biasthen hardcodedntest=1when callingchange_energy_bias_lower. As a result the bias was always estimated from a single frame per system regardless of-n/--numb-batch, and the documented0 means all databehavior had no effect.Fix
Thread
numb_batchinto_apply_data_based_biasand forward it tochange_energy_bias_lowerasntest. Inchange_energy_bias_lower, treatntest <= 0as "use all frames in the system" (numb_test = nframes if ntest <= 0 else min(nframes, ntest)), implementing the documented0 means all data. This is backward compatible: the defaultntestis 10 and no existing caller passed a non-positive value.Test
There was no test covering
numb_batchorchange_energy_bias_lower. A new test builds a small multi-frameDeepmdDataSystemand a mock evaluator that records how many frames it is asked to predict, then asserts thatntest=1evaluates one frame per system andntest=0evaluates all frames. On the current codentest=0selects zero frames (raising downstream); after the fix it uses all frames.The
numb_batch -> ntestparameter plumbing is simple and verified by inspection; the test targets the frame-selection semantics that the fix makes controllable.Fix #5684
Summary by CodeRabbit