Skip to content

fix(tf): honor change-bias --numb-batch#5703

Merged
njzjz merged 1 commit into
deepmodeling:masterfrom
wanghan-iapcm:fix-tf-change-bias-numb-batch
Jul 2, 2026
Merged

fix(tf): honor change-bias --numb-batch#5703
njzjz merged 1 commit into
deepmodeling:masterfrom
wanghan-iapcm:fix-tf-change-bias-numb-batch

Conversation

@wanghan-iapcm

@wanghan-iapcm wanghan-iapcm commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Problem

change-bias --numb-batch is documented as the number of frames to use per data system, with 0 meaning all data. On the TensorFlow data-based path this value was ignored: _change_bias_checkpoint_file called _apply_data_based_bias(...) without numb_batch, and _apply_data_based_bias then hardcoded ntest=1 when calling change_energy_bias_lower. As a result the bias was always estimated from a single frame per system regardless of -n/--numb-batch, and the documented 0 means all data behavior had no effect.

Fix

Thread numb_batch into _apply_data_based_bias and forward it to change_energy_bias_lower as ntest. In change_energy_bias_lower, treat ntest <= 0 as "use all frames in the system" (numb_test = nframes if ntest <= 0 else min(nframes, ntest)), implementing the documented 0 means all data. This is backward compatible: the default ntest is 10 and no existing caller passed a non-positive value.

Test

There was no test covering numb_batch or change_energy_bias_lower. A new test builds a small multi-frame DeepmdDataSystem and a mock evaluator that records how many frames it is asked to predict, then asserts that ntest=1 evaluates one frame per system and ntest=0 evaluates all frames. On the current code ntest=0 selects zero frames (raising downstream); after the fix it uses all frames.

The numb_batch -> ntest parameter plumbing is simple and verified by inspection; the test targets the frame-selection semantics that the fix makes controllable.

Fix #5684

Summary by CodeRabbit

  • Bug Fixes
    • Fixed bias recalculation so batch settings now use the expected number of frames.
    • When no batch limit is set, all available frames are now included instead of a smaller default subset.
  • Tests
    • Added coverage for batch selection behavior to ensure bias updates use the correct frame counts.

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
@wanghan-iapcm wanghan-iapcm requested a review from njzjz July 1, 2026 08:29
@dosubot dosubot Bot added the bug label Jul 1, 2026
@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The TensorFlow change-bias entrypoint now forwards the numb_batch parameter to _apply_data_based_bias, which passes it as ntest to change_energy_bias_lower. change_energy_bias_lower treats ntest <= 0 as using all frames instead of always capping at 1. A new unit test validates this behavior.

Changes

numb_batch threading fix

Layer / File(s) Summary
Thread numb_batch through change-bias entrypoint
deepmd/tf/entrypoints/change_bias.py
change_bias forwards numb_batch to _apply_data_based_bias, whose signature/docstring now accept it and pass it as ntest to change_energy_bias_lower instead of hard-coded 1.
Fix frame-count selection logic
deepmd/tf/fit/ener.py
change_energy_bias_lower now sets numb_test = nframes when ntest <= 0, rather than always computing min(nframes, ntest).
Add unit test for numb_batch frame selection
source/tests/tf/test_change_energy_bias.py
New test module with a _MockDP predictor stub and TestChangeEnergyBiasNumbTest verifying ntest caps evaluated frames and ntest=0 uses all frames.

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
Loading

Suggested labels: Python

Suggested reviewers: njzjz, iProzd

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately describes the main change to TensorFlow change-bias numb-batch handling.
Linked Issues check ✅ Passed The PR matches #5684 by threading numb_batch through the data-based path and preserving 0 as all frames.
Out of Scope Changes check ✅ Passed The code changes and new test are directly related to the numb-batch fix and do not introduce unrelated scope.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
deepmd/tf/fit/ener.py (1)

1028-1029: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Logic fix correct; docstring for ntest should mention the <=0 semantics.

The frame-selection fix is correct. Consider updating the ntest docstring (Lines 1028-1029) to state that ntest <= 0 means "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

📥 Commits

Reviewing files that changed from the base of the PR and between b22a3c3 and 913b93a.

📒 Files selected for processing (3)
  • deepmd/tf/entrypoints/change_bias.py
  • deepmd/tf/fit/ener.py
  • source/tests/tf/test_change_energy_bias.py

@github-actions github-actions Bot added the Python label Jul 1, 2026
@codecov

codecov Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.70%. Comparing base (0e5c170) to head (913b93a).
⚠️ Report is 2 commits behind head on master.

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.
📢 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.

@njzjz-bot njzjz-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@njzjz njzjz added this pull request to the merge queue Jul 1, 2026
Merged via the queue into deepmodeling:master with commit 0f19772 Jul 2, 2026
60 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Code scan] Honor TensorFlow change-bias --numb-batch

3 participants