Skip to content

fix(tf): keep linear-model loss configs after frozen components#5701

Merged
njzjz merged 1 commit into
deepmodeling:masterfrom
wanghan-iapcm:fix-tf-linear-get-loss
Jul 1, 2026
Merged

fix(tf): keep linear-model loss configs after frozen components#5701
njzjz merged 1 commit into
deepmodeling:masterfrom
wanghan-iapcm:fix-tf-linear-get-loss

Conversation

@wanghan-iapcm

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

Copy link
Copy Markdown
Collaborator

Problem

LinearModel.get_loss() reassigned its loss argument to each submodel's returned value:

for model in self.models:
    loss = model.get_loss(loss, lr)
    if loss is not None:
        return loss

Frozen and pairtab submodels return None from get_loss() because they own no trainable loss. When such a non-trainable submodel is ordered before a trainable energy submodel, the trainable submodel is then called with loss=None, and EnerFitting.get_loss() dereferences it as a dict via loss.pop("type", ...), raising AttributeError: 'NoneType' object has no attribute 'pop' during trainer construction — even though the original loss config could have been used.

Fix

Iterate with a separate variable and pass a copy.deepcopy(loss) of the original loss configuration to each submodel, returning the first non-None result. This keeps the caller's loss config intact for the trainable submodel regardless of submodel ordering, and avoids mutating the shared config (EnerFitting.get_loss pops keys from it).

Test

source/tests/tf/test_linear_model.py covered a linear model whose submodels never hit this ordering, so the crash path was unexercised. A new lightweight TestLinearModelGetLoss constructs a LinearEnergyModel with a None-returning submodel (mimicking frozen/pairtab) ordered before a trainable submodel (mimicking EnerFitting.get_loss's loss.pop), and asserts get_loss returns the trainable loss and leaves the original config unmutated. On the current code it fails with the NoneType ... pop error; after the fix it passes.

Fix #5682

Summary by CodeRabbit

  • Bug Fixes

    • Preserves the original loss configuration when evaluating multiple submodels.
    • Ensures a non-trainable submodel returning no loss does not affect later trainable submodels.
  • Tests

    • Added coverage for loss handling across submodels, including unchanged input configuration and correct fallback behavior.

LinearModel.get_loss reassigned its `loss` argument to each submodel's
return value. Frozen and pairtab submodels return None from get_loss, so when
such a non-trainable submodel preceded a trainable energy submodel, the next
submodel was called with loss=None and EnerFitting.get_loss crashed on
None.pop("type").

Iterate with a separate variable and pass a deepcopy of the original loss
config to each submodel, returning the first non-None result. Add a test with
a None-returning submodel ordered before a trainable one.

Fix deepmodeling#5682
@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

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 1eb801a2-96e4-4670-8f0e-b2fa22a5502c

📥 Commits

Reviewing files that changed from the base of the PR and between 0e5c170 and 1de75cd.

📒 Files selected for processing (2)
  • deepmd/tf/model/linear.py
  • source/tests/tf/test_linear_model.py

📝 Walkthrough

Walkthrough

LinearModel.get_loss() in the TensorFlow linear model now deep-copies the loss configuration for each submodel invocation instead of reusing a shared reference, returning the first non-None submodel loss. A new unit test verifies the original loss config remains unmutated after the call.

Changes

Preserve loss config across linear model submodels

Layer / File(s) Summary
Deep-copy loss config per submodel
deepmd/tf/model/linear.py
Imports copy and updates get_loss() to pass copy.deepcopy(loss) to each submodel, keeping the original config intact and returning the first non-None submodel loss.
Test verifying config preservation
source/tests/tf/test_linear_model.py
Adds unittest import and a TestLinearModelGetLoss test case with stub submodels (one returning None, one mutating and returning a loss) asserting get_loss returns the non-None result and leaves the original loss_config dict unchanged.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant LinearModel
  participant FrozenSubmodel
  participant EnergySubmodel
  Caller->>LinearModel: get_loss(loss, lr)
  LinearModel->>FrozenSubmodel: get_loss(copy.deepcopy(loss), lr)
  FrozenSubmodel-->>LinearModel: None
  LinearModel->>EnergySubmodel: get_loss(copy.deepcopy(loss), lr)
  EnergySubmodel-->>LinearModel: loss_result
  LinearModel-->>Caller: loss_result
Loading
🚥 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 clearly summarizes the main fix for preserving linear-model loss configs across frozen components.
Linked Issues check ✅ Passed The code deep-copies loss per submodel and returns the first non-None result, matching issue #5682's fix.
Out of Scope Changes check ✅ Passed The changes stay focused on the loss-handling bug and its regression test, with no unrelated additions.
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.

@github-actions github-actions Bot added the Python label Jul 1, 2026
@wanghan-iapcm wanghan-iapcm requested a review from njzjz July 1, 2026 08:11
@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.78%. Comparing base (0e5c170) to head (1de75cd).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5701      +/-   ##
==========================================
- Coverage   81.97%   81.78%   -0.19%     
==========================================
  Files         959      959              
  Lines      105748   105749       +1     
  Branches     4102     4106       +4     
==========================================
- Hits        86684    86486     -198     
- Misses      17573    17769     +196     
- Partials     1491     1494       +3     

☔ 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 change preserves the original loss configuration for each linear submodel, correctly skips non-trainable submodels returning None, and adds a focused regression test for the frozen/pairtab-before-trainable ordering. CI is green.

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 d0e531d Jul 1, 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] Keep TensorFlow linear-model loss configs after frozen components

3 participants