fix(pt): address hessian review comments#5358
Closed
njzjz-bot wants to merge 2 commits intodeepmodeling:masterfrom
Closed
fix(pt): address hessian review comments#5358njzjz-bot wants to merge 2 commits intodeepmodeling:masterfrom
njzjz-bot wants to merge 2 commits intodeepmodeling:masterfrom
Conversation
Use vector_norm semantics in safe_for_norm and add focused regression tests to verify DPA2/DPA3 Hessians stay finite. Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.4)
Contributor
📝 WalkthroughWalkthroughThis PR introduces a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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: 1
🧹 Nitpick comments (1)
deepmd/pt/model/descriptor/repflows.py (1)
479-479: Compute norm only for the sliced neighbor window to avoid extra work.Line 479 computes norms over all neighbors and slices afterward. You can slice first and reduce unnecessary FLOPs when
self.a_sel < nnei.♻️ Suggested change
- a_dist_mask = (safe_for_norm(diff, dim=-1) < self.a_rcut)[:, :, : self.a_sel] + a_dist_mask = ( + safe_for_norm(diff[:, :, : self.a_sel], dim=-1) < self.a_rcut + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deepmd/pt/model/descriptor/repflows.py` at line 479, The current code computes norms over the full neighbor set then slices, causing extra work; instead slice the neighbor window first and compute safe_for_norm on the sliced tensor so a_dist_mask is computed from the norms of diff[:, :, :self.a_sel] only — i.e., take diff_window = diff[:, :, :self.a_sel], run safe_for_norm(diff_window, dim=-1) and compare with self.a_rcut to produce a_dist_mask; update any downstream uses that assumed full-length tensors accordingly (symbols: a_dist_mask, safe_for_norm, diff, self.a_rcut, self.a_sel, nnei).
🤖 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/utils/safe_gradient.py`:
- Line 22: Rename the parameter named "ord" in the function in
deepmd/pt/utils/safe_gradient.py to a non-builtin name (e.g., "norm_ord" or "p")
to avoid shadowing Python's ord builtin; update the function signature and every
use of that parameter within the function (and any internal helper closures) to
the new name, and propagate the rename to any local callers in the same module
so references remain consistent.
---
Nitpick comments:
In `@deepmd/pt/model/descriptor/repflows.py`:
- Line 479: The current code computes norms over the full neighbor set then
slices, causing extra work; instead slice the neighbor window first and compute
safe_for_norm on the sliced tensor so a_dist_mask is computed from the norms of
diff[:, :, :self.a_sel] only — i.e., take diff_window = diff[:, :, :self.a_sel],
run safe_for_norm(diff_window, dim=-1) and compare with self.a_rcut to produce
a_dist_mask; update any downstream uses that assumed full-length tensors
accordingly (symbols: a_dist_mask, safe_for_norm, diff, self.a_rcut, self.a_sel,
nnei).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: df7a4b50-c848-4af4-b8b8-77c7b3426e24
📒 Files selected for processing (4)
deepmd/pt/model/descriptor/repflows.pydeepmd/pt/model/descriptor/repformers.pydeepmd/pt/utils/safe_gradient.pysource/tests/pt/model/test_dpa_hessian_finite.py
| x: torch.Tensor, | ||
| dim: int | None = None, | ||
| keepdim: bool = False, | ||
| ord: float = 2.0, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ruff check deepmd/pt/utils/safe_gradient.py
ruff format --check deepmd/pt/utils/safe_gradient.pyRepository: deepmodeling/deepmd-kit
Length of output: 578
Rename ord argument to avoid Ruff A002 error.
Line 22 shadows Python's builtin ord, which Ruff flags as A002. This will cause CI to fail per the coding guidelines.
🔧 Proposed fix
def safe_for_norm(
x: torch.Tensor,
dim: int | None = None,
keepdim: bool = False,
- ord: float = 2.0,
+ norm_ord: float = 2.0,
) -> torch.Tensor:
@@
- norm = torch.linalg.vector_norm(x_safe, ord=ord)
+ norm = torch.linalg.vector_norm(x_safe, ord=norm_ord)
@@
- norm = torch.linalg.vector_norm(x_safe, ord=ord, dim=dim, keepdim=keepdim)
+ norm = torch.linalg.vector_norm(x_safe, ord=norm_ord, dim=dim, keepdim=keepdim)🧰 Tools
🪛 Ruff (0.15.7)
[error] 22-22: Function argument ord is shadowing a Python builtin
(A002)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deepmd/pt/utils/safe_gradient.py` at line 22, Rename the parameter named
"ord" in the function in deepmd/pt/utils/safe_gradient.py to a non-builtin name
(e.g., "norm_ord" or "p") to avoid shadowing Python's ord builtin; update the
function signature and every use of that parameter within the function (and any
internal helper closures) to the new name, and propagate the rename to any local
callers in the same module so references remain consistent.
Contributor
Author
|
Closing this PR because the intended target is Superseded by: njzjz#227 — OpenClaw 2026.3.8 (model: custom-chat-jinzhezeng-group/gpt-5.4) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5358 +/- ##
==========================================
- Coverage 82.26% 82.18% -0.09%
==========================================
Files 799 811 +12
Lines 82563 83237 +674
Branches 4066 4066
==========================================
+ Hits 67924 68410 +486
- Misses 13424 13611 +187
- Partials 1215 1216 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
2 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.
Problem
safe_for_normmatches vector-norm semantics whendimis not provided.Change
torch.linalg.vector_norm(...)insafe_for_normfor bothdim=Noneand dimensioned calls.repformer.direct_dist=True) and DPA3 Hessians remain finite.Notes
Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.4)
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests