chore(pt): mv the input stat update to model_change_out_bias#5266
Queued
wanghan-iapcm wants to merge 2 commits intodeepmodeling:masterfrom
Queued
chore(pt): mv the input stat update to model_change_out_bias#5266wanghan-iapcm wants to merge 2 commits intodeepmodeling:masterfrom
wanghan-iapcm wants to merge 2 commits intodeepmodeling:masterfrom
Conversation
…the logic of change_out_bias clean
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f599b6a696
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
📝 WalkthroughWalkthroughRemoved the statistic computation from model-layer Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
source/tests/pt/test_training.py (1)
699-699: Nit: replace lambda assignment with a named function to avoidnoqasuppression.♻️ Proposed refactor
- sample_func = lambda: merged # noqa: E731 + def sample_func() -> list: + return merged🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/tests/pt/test_training.py` at line 699, Replace the lambda assigned to sample_func with a proper named function to avoid the noqa suppression: change the assignment "sample_func = lambda: merged" to a function definition like "def sample_func(): return merged" (or equivalent) and remove the "# noqa: E731" comment; update any references to sample_func to use the new function as before.deepmd/pt/train/training.py (1)
1758-1763: Hoist the import to improve code organization.The import path is correct and PT models (EnergyModel, PolarModel, PropertyModel, etc.) properly inherit from
DPModelCommonindeepmd/pt/model/model/dp_model.py, so the isinstance check works as intended. There is no circular import concern:deepmd/pt/train/training.pyimports fromdeepmd.pt.model.model, but the model package does not import from training.The deferred import sits awkwardly mid-function between unrelated statements. Moving it to the module level or to the top of
model_change_out_bias()would improve readability without risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deepmd/pt/train/training.py` around lines 1758 - 1763, The local import of DPModelCommon should be hoisted out of the middle of the function: move the "from deepmd.pt.model.model.dp_model import DPModelCommon" to the module top (or at the start of model_change_out_bias()) and remove the inline import; keep the existing isinstance(_model, DPModelCommon) and the conditional block that calls _model.get_fitting_net().compute_input_stats(_sample_func) unchanged so the logic using _bias_adjust_mode, _model, compute_input_stats, and _sample_func still runs the same after the import is relocated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@deepmd/pt/train/training.py`:
- Around line 1758-1763: The local import of DPModelCommon should be hoisted out
of the middle of the function: move the "from deepmd.pt.model.model.dp_model
import DPModelCommon" to the module top (or at the start of
model_change_out_bias()) and remove the inline import; keep the existing
isinstance(_model, DPModelCommon) and the conditional block that calls
_model.get_fitting_net().compute_input_stats(_sample_func) unchanged so the
logic using _bias_adjust_mode, _model, compute_input_stats, and _sample_func
still runs the same after the import is relocated.
In `@source/tests/pt/test_training.py`:
- Line 699: Replace the lambda assigned to sample_func with a proper named
function to avoid the noqa suppression: change the assignment "sample_func =
lambda: merged" to a function definition like "def sample_func(): return merged"
(or equivalent) and remove the "# noqa: E731" comment; update any references to
sample_func to use the new function as before.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
deepmd/pt/model/model/make_model.pydeepmd/pt/train/training.pysource/tests/pt/test_training.py
💤 Files with no reviewable changes (1)
- deepmd/pt/model/model/make_model.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
deepmd/pd/train/training.py (1)
1351-1356: Move the deferred import inside theifguard.The import of
DPModelCommonis currently executed unconditionally on every call tomodel_change_out_bias, even when_bias_adjust_mode != "set-by-statistic". Placing it inside the conditional keeps the import tied to the only code path that actually needs it and matches the intent of a lazy/deferred import.♻️ Proposed refactor
- from deepmd.pd.model.model.dp_model import ( - DPModelCommon, - ) - - if isinstance(_model, DPModelCommon) and _bias_adjust_mode == "set-by-statistic": + if _bias_adjust_mode == "set-by-statistic": + from deepmd.pd.model.model.dp_model import ( + DPModelCommon, + ) + if _bias_adjust_mode == "set-by-statistic" and isinstance(_model, DPModelCommon): _model.get_fitting_net().compute_input_stats(_sample_func)Or more concisely:
- from deepmd.pd.model.model.dp_model import ( - DPModelCommon, - ) - - if isinstance(_model, DPModelCommon) and _bias_adjust_mode == "set-by-statistic": - _model.get_fitting_net().compute_input_stats(_sample_func) + if _bias_adjust_mode == "set-by-statistic": + from deepmd.pd.model.model.dp_model import DPModelCommon + if isinstance(_model, DPModelCommon): + _model.get_fitting_net().compute_input_stats(_sample_func)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deepmd/pd/train/training.py` around lines 1351 - 1356, Move the deferred import of DPModelCommon into the conditional so it only happens when needed: inside the model_change_out_bias flow, replace the current top-level import and unconditional isinstance check with an if _bias_adjust_mode == "set-by-statistic": from deepmd.pd.model.model.dp_model import DPModelCommon; if isinstance(_model, DPModelCommon): _model.get_fitting_net().compute_input_stats(_sample_func). This ensures the DPModelCommon import occurs only when the "set-by-statistic" path executes and preserves the existing call to get_fitting_net().compute_input_stats(_sample_func).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@deepmd/pd/train/training.py`:
- Around line 1351-1356: Move the deferred import of DPModelCommon into the
conditional so it only happens when needed: inside the model_change_out_bias
flow, replace the current top-level import and unconditional isinstance check
with an if _bias_adjust_mode == "set-by-statistic": from
deepmd.pd.model.model.dp_model import DPModelCommon; if isinstance(_model,
DPModelCommon): _model.get_fitting_net().compute_input_stats(_sample_func). This
ensures the DPModelCommon import occurs only when the "set-by-statistic" path
executes and preserves the existing call to
get_fitting_net().compute_input_stats(_sample_func).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
deepmd/pd/model/model/make_model.pydeepmd/pd/train/training.pysource/tests/pd/test_training.py
💤 Files with no reviewable changes (1)
- deepmd/pd/model/model/make_model.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5266 +/- ##
==========================================
- Coverage 82.12% 82.00% -0.13%
==========================================
Files 740 750 +10
Lines 74473 75083 +610
Branches 3616 3615 -1
==========================================
+ Hits 61162 61571 +409
- Misses 12149 12347 +198
- Partials 1162 1165 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Chengqian-Zhang
approved these changes
Feb 25, 2026
iProzd
approved these changes
Feb 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
cleanup the logic of change_out_bias: it only change the output bias, does not update the fitting input stat.
Summary by CodeRabbit
Refactor
Tests