feat(dp, pt): add force l2 norm loss & mae loss#5294
Open
iProzd wants to merge 4 commits intodeepmodeling:masterfrom
Open
feat(dp, pt): add force l2 norm loss & mae loss#5294iProzd wants to merge 4 commits intodeepmodeling:masterfrom
iProzd wants to merge 4 commits intodeepmodeling:masterfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds two flags to energy loss code: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deepmd/pt/loss/ener.py`:
- Around line 56-60: The constructor added new state flags (use_mae_loss,
f_use_norm, use_huber, huber_delta) but serialize() does not emit them, so
deserialization resets behavior; update the class's serialize() to include these
fields (use_mae_loss, f_use_norm, use_huber, huber_delta) in the serialized
dict/object and also ensure the corresponding deserialization/load path reads
them back into the instance (same names as the constructor args) so the loss
behavior is preserved after reload; reference the serialize() method and the
constructor parameter names when making the change.
In `@source/tests/consistent/loss/test_ener.py`:
- Around line 88-95: In skip_tf and skip_pd properties replace the full tuple
unpacking that creates unused locals with wildcard placeholders so Ruff won't
flag RUF059; locate the properties skip_tf and skip_pd and change the unpacking
from "(use_huber, enable_atom_ener_coeff, use_mae_loss, f_use_norm) =
self.param" to only bind the used elements (e.g. "(_, _, use_mae_loss,
f_use_norm) = self.param" or similar) so only use_mae_loss and f_use_norm are
named.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4125c703-fe2c-4e34-b371-8b1aa866ee9a
📒 Files selected for processing (4)
deepmd/dpmodel/loss/ener.pydeepmd/pt/loss/ener.pydeepmd/utils/argcheck.pysource/tests/consistent/loss/test_ener.py
Comment on lines
+56
to
60
| use_mae_loss: bool = False, | ||
| inference: bool = False, | ||
| use_huber: bool = False, | ||
| f_use_norm: bool = False, | ||
| huber_delta: float = 0.01, |
Contributor
There was a problem hiding this comment.
Persist new flags in serialize() to prevent silent behavior changes after reload.
use_mae_loss and f_use_norm are now constructor state, but they are not emitted in serialize(). After deserialization, both revert to defaults, changing loss behavior unexpectedly.
💡 Suggested patch
def serialize(self) -> dict:
@@
return {
"@class": "EnergyLoss",
"@version": 2,
@@
"use_huber": self.use_huber,
"huber_delta": self.huber_delta,
+ "use_mae_loss": self.use_mae_loss,
+ "f_use_norm": self.f_use_norm,
}Also applies to: 147-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deepmd/pt/loss/ener.py` around lines 56 - 60, The constructor added new state
flags (use_mae_loss, f_use_norm, use_huber, huber_delta) but serialize() does
not emit them, so deserialization resets behavior; update the class's
serialize() to include these fields (use_mae_loss, f_use_norm, use_huber,
huber_delta) in the serialized dict/object and also ensure the corresponding
deserialization/load path reads them back into the instance (same names as the
constructor args) so the loss behavior is preserved after reload; reference the
serialize() method and the constructor parameter names when making the change.
Comment on lines
+88
to
+95
| (use_huber, enable_atom_ener_coeff, use_mae_loss, f_use_norm) = self.param | ||
| # Skip TF for MAE loss tests (not implemented in TF backend) | ||
| return CommonTest.skip_tf or use_mae_loss or f_use_norm | ||
|
|
||
| @property | ||
| def skip_pd(self) -> bool: | ||
| (use_huber, enable_atom_ener_coeff, use_mae_loss, f_use_norm) = self.param | ||
| # Skip Paddle for MAE loss tests (not implemented in Paddle backend) |
Contributor
There was a problem hiding this comment.
Fix unused unpacked variables to satisfy Ruff (RUF059).
A few tuple-unpacked values are never used in skip/setup paths.
✂️ Suggested cleanup
- (use_huber, enable_atom_ener_coeff, use_mae_loss, f_use_norm) = self.param
+ (_, _, use_mae_loss, f_use_norm) = self.param
@@
- (use_huber, enable_atom_ener_coeff, use_mae_loss, f_use_norm) = self.param
+ (_, _, use_mae_loss, f_use_norm) = self.param
@@
- (use_huber, enable_atom_ener_coeff, use_mae_loss, f_use_norm) = self.param
+ (use_huber, _enable_atom_ener_coeff, use_mae_loss, f_use_norm) = self.paramAs per coding guidelines **/*.py: Always run ruff check . and ruff format . before committing changes or CI will fail.
Also applies to: 111-111
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 88-88: Unpacked variable use_huber is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 88-88: Unpacked variable enable_atom_ener_coeff is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 94-94: Unpacked variable use_huber is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 94-94: Unpacked variable enable_atom_ener_coeff is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/tests/consistent/loss/test_ener.py` around lines 88 - 95, In skip_tf
and skip_pd properties replace the full tuple unpacking that creates unused
locals with wildcard placeholders so Ruff won't flag RUF059; locate the
properties skip_tf and skip_pd and change the unpacking from "(use_huber,
enable_atom_ener_coeff, use_mae_loss, f_use_norm) = self.param" to only bind the
used elements (e.g. "(_, _, use_mae_loss, f_use_norm) = self.param" or similar)
so only use_mae_loss and f_use_norm are named.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5294 +/- ##
==========================================
+ Coverage 82.31% 82.32% +0.01%
==========================================
Files 768 768
Lines 77056 77097 +41
Branches 3660 3659 -1
==========================================
+ Hits 63428 63472 +44
+ Misses 12458 12453 -5
- Partials 1170 1172 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deepmd/pd/loss/ener.py`:
- Around line 60-61: The code stores the f_use_norm flag but silently ignores it
in the force-loss computation; add a fail-fast check for f_use_norm in the
EnerLoss class (constructor or at start of forward()) so that if f_use_norm is
True you raise a clear exception (e.g. NotImplementedError or ValueError)
explaining vector-norm force loss is not implemented, instead of silently
proceeding; update all places that set or pass f_use_norm (the constructor
signature and where it is stored) to ensure the flag triggers this check, and
ensure the error message references f_use_norm and suggests implementing
vector-norm force loss or setting f_use_norm=False.
In `@deepmd/tf/loss/ener.py`:
- Around line 152-155: The TF backend currently stores use_mae_loss and
f_use_norm on self but build() never uses them, so accept-but-ignore can mislead
users; update the class constructor (where self.use_huber, self.huber_delta,
self.use_mae_loss, self.f_use_norm are set) to validate and explicitly reject
unsupported flags: if use_mae_loss or f_use_norm is True, raise a ValueError (or
TypeError) with a clear message referencing the TF backend and the unsupported
options; mention the symbols use_mae_loss, f_use_norm and the build() method in
the error/message so callers know these flags are not implemented in the TF
implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 62d5f40b-7bc8-4e83-846a-cc5d6e489ca0
📒 Files selected for processing (2)
deepmd/pd/loss/ener.pydeepmd/tf/loss/ener.py
Comment on lines
+60
to
+61
| f_use_norm: bool = False, | ||
| use_mae_loss: bool | None = None, |
Contributor
There was a problem hiding this comment.
Fail fast when f_use_norm=True in the PD backend.
Line 158 stores f_use_norm, but forward() still computes force loss component-wise and never switches to a vector-norm formulation. Right now f_use_norm=True is silently ignored.
Proposed fix
self.use_l1_all = use_l1_all
self.inference = inference
self.use_huber = use_huber
self.huber_delta = huber_delta
self.f_use_norm = f_use_norm
+ if self.f_use_norm:
+ raise RuntimeError("f_use_norm is not implemented in the PD backend.")
if self.use_huber and (
self.has_pf or self.has_gf or self.relative_f is not None
):Also applies to: 114-119, 158-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deepmd/pd/loss/ener.py` around lines 60 - 61, The code stores the f_use_norm
flag but silently ignores it in the force-loss computation; add a fail-fast
check for f_use_norm in the EnerLoss class (constructor or at start of
forward()) so that if f_use_norm is True you raise a clear exception (e.g.
NotImplementedError or ValueError) explaining vector-norm force loss is not
implemented, instead of silently proceeding; update all places that set or pass
f_use_norm (the constructor signature and where it is stored) to ensure the flag
triggers this check, and ensure the error message references f_use_norm and
suggests implementing vector-norm force loss or setting f_use_norm=False.
Comment on lines
152
to
+155
| self.use_huber = use_huber | ||
| self.huber_delta = huber_delta | ||
| self.use_mae_loss = use_mae_loss | ||
| self.f_use_norm = f_use_norm |
Contributor
There was a problem hiding this comment.
Reject unsupported MAE / force-norm flags in the TF backend.
Lines 154-155 persist use_mae_loss and f_use_norm, but build() never reads either flag. Passing either option will silently fall back to the existing L2/Huber behavior, which makes the config lie about the loss being optimized.
Proposed fix
self.use_huber = use_huber
self.huber_delta = huber_delta
self.use_mae_loss = use_mae_loss
self.f_use_norm = f_use_norm
+ if self.use_mae_loss or self.f_use_norm:
+ raise RuntimeError(
+ "use_mae_loss and f_use_norm are not implemented in the TF backend."
+ )
if self.use_huber and (
self.has_pf or self.has_gf or self.relative_f is not None
):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deepmd/tf/loss/ener.py` around lines 152 - 155, The TF backend currently
stores use_mae_loss and f_use_norm on self but build() never uses them, so
accept-but-ignore can mislead users; update the class constructor (where
self.use_huber, self.huber_delta, self.use_mae_loss, self.f_use_norm are set) to
validate and explicitly reject unsupported flags: if use_mae_loss or f_use_norm
is True, raise a ValueError (or TypeError) with a clear message referencing the
TF backend and the unsupported options; mention the symbols use_mae_loss,
f_use_norm and the build() method in the error/message so callers know these
flags are not implemented in the TF implementation.
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
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.
Summary by CodeRabbit
New Features
Tests
Documentation
Chores