feat(dp, pt): add charge spin embedding#5295
Merged
njzjz merged 9 commits intodeepmodeling:masterfrom Mar 13, 2026
Merged
Conversation
Signed-off-by: Duo <50307526+iProzd@users.noreply.github.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an optional frame parameter fparam and a boolean flag add_chg_spin_ebd across descriptor and atomic model layers; when enabled, DPA3 builds charge/spin embeddings and mixes them into node embeddings. fparam is propagated through call/forward signatures and test harnesses; atomic models may synthesize default fparam per frame. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AtomicModel as DPAtomicModel
participant Descriptor as DescrptDPA3
participant ChgEmbed as chg_embedding
participant SpinEmbed as spin_embedding
participant MixMLP as mix_cs_mlp
User->>AtomicModel: forward(coords, fparam=None)
alt add_chg_spin_ebd enabled and fparam not provided
AtomicModel->>AtomicModel: build default fparam (tile per frame)
end
AtomicModel->>Descriptor: call(..., fparam=fparam_input if add_chg_spin_ebd else None)
alt add_chg_spin_ebd enabled and fparam provided
Descriptor->>ChgEmbed: compute chg_ebd
Descriptor->>SpinEmbed: compute spin_ebd
ChgEmbed-->>Descriptor: chg_ebd
SpinEmbed-->>Descriptor: spin_ebd
Descriptor->>MixMLP: forward(concat(chg_ebd, spin_ebd))
MixMLP-->>Descriptor: mixed_cs
Descriptor->>Descriptor: add mixed_cs to node_ebd_ext
end
Descriptor-->>AtomicModel: descriptor outputs
AtomicModel-->>User: predictions
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)
📝 Coding Plan
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 Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
deepmd/dpmodel/descriptor/dpa2.py (1)
825-864:⚠️ Potential issue | 🟠 MajorConsume or discard
fparamto keep lint green.Line 831 introduces
fparam, but it is unused incall, which triggers ARG002. If unused by design, explicitly discard it.💡 Minimal fix
xp = array_api_compat.array_namespace(coord_ext, atype_ext, nlist) + del fparam use_three_body = self.use_three_bodyAs per coding guidelines
**/*.py: Always runruff check .andruff format .before committing changes or CI will fail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deepmd/dpmodel/descriptor/dpa2.py` around lines 825 - 864, The parameter fparam in the method call of class DPA2 (function name: call) is currently unused which triggers ARG002; explicitly consume or discard it to satisfy the linter — for example, add a single statement that references fparam (e.g., _ = fparam or del fparam) near the top of call after xp is set, ensuring the symbol is acknowledged but no behavior changes occur; keep the change minimal and run ruff check/format before committing.deepmd/dpmodel/descriptor/se_t_tebd.py (1)
349-388:⚠️ Potential issue | 🟠 MajorDiscard unused
fparamto avoid ARG002.Line 355 adds
fparam, but the method does not use it. Explicitly deleting it keeps API compatibility and lint compliance.💡 Minimal fix
- del mapping + del mapping, fparamAs per coding guidelines
**/*.py: Always runruff check .andruff format .before committing changes or CI will fail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deepmd/dpmodel/descriptor/se_t_tebd.py` around lines 349 - 388, The method call in class/function se_t_tebd (function name: call) declares parameter fparam but never uses it, causing ARG002; fix by explicitly discarding the unused parameter (e.g., add "del fparam" or assign to "_" at the start of the function alongside the existing "del mapping") to keep API compatibility and satisfy linters; ensure you place this discard near the existing "del mapping" line so the signature remains unchanged while removing the unused-variable warning.deepmd/dpmodel/descriptor/hybrid.py (1)
272-336:⚠️ Potential issue | 🔴 CriticalForward
fparamto child descriptors; it is currently dropped.Line 278 adds
fparam, but Line 335 does not pass it downstream. This breaks charge/spin embedding paths for child descriptors (e.g., DPA3) that require frame parameters.🔧 Proposed fix
- odescriptor, gr, g2, h2, sw = descrpt(coord_ext, atype_ext, nl, mapping) + odescriptor, gr, g2, h2, sw = descrpt( + coord_ext, + atype_ext, + nl, + mapping, + fparam=fparam, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deepmd/dpmodel/descriptor/hybrid.py` around lines 272 - 336, The hybrid descriptor's call method accepts fparam but doesn't forward it into child descriptor calls: in method call (symbols: fparam, descrpt, descrpt_list) ensure the child invocation descrpt(coord_ext, atype_ext, nl, mapping) is changed to pass the frame params (e.g., descrpt(coord_ext, atype_ext, nl, mapping, fparam) or the correct child API) so fparam is propagated to descriptors like DPA3; update all places in this method where descrpt(...) is invoked to include fparam while preserving backward compatibility if some children do not accept it.deepmd/pt/model/descriptor/se_t.py (1)
339-391:⚠️ Potential issue | 🟠 MajorExplicitly discard unused
fparamto prevent ARG002.Line 346 adds
fparam, but it is currently unused. Please consume or explicitly delete it to keep Ruff clean.💡 Minimal fix
# cast the input to internal precsion coord_ext = coord_ext.to(dtype=self.prec) + del fparam g1, rot_mat, g2, h2, sw = self.seat.forward( nlist, coord_ext, atype_ext, None, mapping )As per coding guidelines
**/*.py: Always runruff check .andruff format .before committing changes or CI will fail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deepmd/pt/model/descriptor/se_t.py` around lines 339 - 391, The forward method currently accepts fparam but never uses it, triggering an ARG002 lint; inside the forward method of se_t (function name forward) explicitly discard the unused parameter (for example assign it to _ or call del fparam) before returning/using other values so Ruff no longer flags it; ensure this change is placed near the start of forward (before calling self.seat.forward) so the parameter is visibly consumed and no behavior changes occur.deepmd/pt/model/descriptor/hybrid.py (1)
264-336:⚠️ Potential issue | 🔴 Critical
fparamis accepted but not propagated to child descriptors.Line 271 adds
fparam, but Line 335 drops it when invoking each sub-descriptor. This can break DPA3 charge/spin embedding flows that require frame parameters.🔧 Proposed fix
- odescriptor, gr, g2, h2, sw = descrpt(coord_ext, atype_ext, nl, mapping) + odescriptor, gr, g2, h2, sw = descrpt( + coord_ext, + atype_ext, + nl, + mapping, + comm_dict=comm_dict, + fparam=fparam, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deepmd/pt/model/descriptor/hybrid.py` around lines 264 - 336, The forward method accepts fparam but does not pass it to sub-descriptors, breaking flows that need frame params; update the loop in Hybrid.forward where each descriptor is invoked (the call to descrpt(...)) to forward comm_dict and fparam as additional arguments (e.g., replace descrpt(coord_ext, atype_ext, nl, mapping) with descrpt(coord_ext, atype_ext, nl, mapping, comm_dict, fparam) or the correct child-descriptor signature) so each element of self.descrpt_list receives the frame parameters; keep the existing logic for nl selection and ensure the passed variables use the same names (comm_dict and fparam).deepmd/pt/model/descriptor/dpa1.py (1)
669-711:⚠️ Potential issue | 🟠 MajorHandle
fparamexplicitly to avoid Ruff ARG002 failures.Line 669 adds
fparam, but it is not consumed in this method. If this descriptor intentionally ignores frame parameters, explicitly discard it so lint stays clean.💡 Minimal fix
- del mapping + del mapping, fparamAs per coding guidelines
**/*.py: Always runruff check .andruff format .before committing changes or CI will fail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deepmd/pt/model/descriptor/dpa1.py` around lines 669 - 711, The parameter fparam is unused and triggers Ruff ARG002; explicitly discard it to satisfy the linter by deleting it where other unused args are removed (near the existing del mapping line). Update the method that begins with the shown signature (accepting extended_coord, extended_atype, nlist, mapping, comm_dict, fparam) to include a statement removing fparam (e.g., del fparam) immediately after or alongside del mapping so the unused-argument warning is resolved.deepmd/pt/model/descriptor/se_r.py (1)
430-473:⚠️ Potential issue | 🟡 MinorDiscard unused
fparaminforwardfor lint compliance.
fparamis accepted for API consistency but currently unused. Please explicitly discard it alongside the other unused inputs.Proposed fix
- del mapping, comm_dict + del mapping, comm_dict, fparamAs per coding guidelines
**/*.py: Always runruff check .andruff format .before committing changes or CI will fail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deepmd/pt/model/descriptor/se_r.py` around lines 430 - 473, The forward method currently accepts fparam but doesn't discard it explicitly; to satisfy linter rules, explicitly discard fparam in the same way mapping and comm_dict are discarded (e.g., add fparam to the existing del statement), referencing the forward signature and the existing del mapping, comm_dict usage so the unused parameter is removed from scope.deepmd/dpmodel/descriptor/se_t.py (1)
347-380:⚠️ Potential issue | 🟡 MinorExplicitly discard unused
fparamto avoid lint failures.At Line 347,
fparamis added but never used or discarded. Mirror the existingmappinghandling so this stays clean under strict Ruff settings.Proposed fix
- del mapping + del mapping, fparamAs per coding guidelines
**/*.py: Always runruff check .andruff format .before committing changes or CI will fail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deepmd/dpmodel/descriptor/se_t.py` around lines 347 - 380, The new parameter fparam in the descriptor method is unused and should be explicitly discarded to satisfy linters; mirror the existing handling of mapping by adding an explicit discard (e.g., del fparam) in the same function (the descriptor method in se_t.py where mapping is deleted and xp is set) so Ruff no longer reports an unused variable.deepmd/dpmodel/descriptor/dpa1.py (1)
496-528:⚠️ Potential issue | 🟡 Minor
fparamis unused incall; explicitly discard it.Line 496 introduces
fparambut the method does not consume it yet. Explicitly deleting it keeps this compatible with Ruff settings and makes intent clear.Proposed fix
- del mapping + del mapping, fparamAs per coding guidelines
**/*.py: Always runruff check .andruff format .before committing changes or CI will fail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deepmd/dpmodel/descriptor/dpa1.py` around lines 496 - 528, The method call currently accepts fparam but never uses it; add an explicit discard for this parameter (e.g., del fparam) near the start of the call method alongside the existing del mapping to make intent clear and satisfy linters/ruff; update the dpa1.py call method (the function that defines fparam in its signature) to explicitly delete fparam so it's not flagged as unused.
🤖 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/dpmodel/descriptor/dpa3.py`:
- Around line 455-460: The seed for mix_cs_mlp is colliding with chg_embedding
because both call child_seed(seed, 3); update the mix_cs_mlp initialization in
the class (the NativeLayer assignment named mix_cs_mlp) to use a distinct
child_seed index (e.g., child_seed(seed, 4) or the next unused index) so each
layer gets a unique seed and avoids deterministic collisions with chg_embedding.
In `@deepmd/pt/model/descriptor/dpa2.py`:
- Line 718: The parameter fparam is declared in the forward signature but never
used, triggering ARG002; keep the parameter for API compatibility but mark it
intentionally unused by referencing it in the method body (e.g., add a single
line like "del fparam" or assign to _ inside the forward method) so Ruff/CI
stops reporting the unused-argument error while leaving the API unchanged;
locate the forward function that accepts "fparam: torch.Tensor | None = None"
and add the one-line no-op reference at the top of that method.
In `@deepmd/pt/model/descriptor/dpa3.py`:
- Around line 214-219: chg_embedding and mix_cs_mlp both call child_seed(seed,
3) causing identical initializations; change the seed index used for mix_cs_mlp
to a unique value (e.g., child_seed(seed, 4) or another unused index) where
mix_cs_mlp is constructed inside the MLPLayer call so that MLPLayer(...,
seed=child_seed(seed, X)) uses a different child index than chg_embedding to
avoid seed collision.
In `@source/tests/consistent/descriptor/test_dpa3.py`:
- Line 162: The tuple element add_chg_spin_ebd is being unpacked but never used
in multiple places; update each unpacking to either remove that element from the
left-hand side or mark it as intentionally ignored (e.g., replace
add_chg_spin_ebd with a single underscore variable name) so Ruff RUF059 no
longer flags it; search for occurrences of add_chg_spin_ebd in test_dpa3 and
change the unpack patterns accordingly, keeping the rest of the unpacking order
intact.
---
Outside diff comments:
In `@deepmd/dpmodel/descriptor/dpa1.py`:
- Around line 496-528: The method call currently accepts fparam but never uses
it; add an explicit discard for this parameter (e.g., del fparam) near the start
of the call method alongside the existing del mapping to make intent clear and
satisfy linters/ruff; update the dpa1.py call method (the function that defines
fparam in its signature) to explicitly delete fparam so it's not flagged as
unused.
In `@deepmd/dpmodel/descriptor/dpa2.py`:
- Around line 825-864: The parameter fparam in the method call of class DPA2
(function name: call) is currently unused which triggers ARG002; explicitly
consume or discard it to satisfy the linter — for example, add a single
statement that references fparam (e.g., _ = fparam or del fparam) near the top
of call after xp is set, ensuring the symbol is acknowledged but no behavior
changes occur; keep the change minimal and run ruff check/format before
committing.
In `@deepmd/dpmodel/descriptor/hybrid.py`:
- Around line 272-336: The hybrid descriptor's call method accepts fparam but
doesn't forward it into child descriptor calls: in method call (symbols: fparam,
descrpt, descrpt_list) ensure the child invocation descrpt(coord_ext, atype_ext,
nl, mapping) is changed to pass the frame params (e.g., descrpt(coord_ext,
atype_ext, nl, mapping, fparam) or the correct child API) so fparam is
propagated to descriptors like DPA3; update all places in this method where
descrpt(...) is invoked to include fparam while preserving backward
compatibility if some children do not accept it.
In `@deepmd/dpmodel/descriptor/se_t_tebd.py`:
- Around line 349-388: The method call in class/function se_t_tebd (function
name: call) declares parameter fparam but never uses it, causing ARG002; fix by
explicitly discarding the unused parameter (e.g., add "del fparam" or assign to
"_" at the start of the function alongside the existing "del mapping") to keep
API compatibility and satisfy linters; ensure you place this discard near the
existing "del mapping" line so the signature remains unchanged while removing
the unused-variable warning.
In `@deepmd/dpmodel/descriptor/se_t.py`:
- Around line 347-380: The new parameter fparam in the descriptor method is
unused and should be explicitly discarded to satisfy linters; mirror the
existing handling of mapping by adding an explicit discard (e.g., del fparam) in
the same function (the descriptor method in se_t.py where mapping is deleted and
xp is set) so Ruff no longer reports an unused variable.
In `@deepmd/pt/model/descriptor/dpa1.py`:
- Around line 669-711: The parameter fparam is unused and triggers Ruff ARG002;
explicitly discard it to satisfy the linter by deleting it where other unused
args are removed (near the existing del mapping line). Update the method that
begins with the shown signature (accepting extended_coord, extended_atype,
nlist, mapping, comm_dict, fparam) to include a statement removing fparam (e.g.,
del fparam) immediately after or alongside del mapping so the unused-argument
warning is resolved.
In `@deepmd/pt/model/descriptor/hybrid.py`:
- Around line 264-336: The forward method accepts fparam but does not pass it to
sub-descriptors, breaking flows that need frame params; update the loop in
Hybrid.forward where each descriptor is invoked (the call to descrpt(...)) to
forward comm_dict and fparam as additional arguments (e.g., replace
descrpt(coord_ext, atype_ext, nl, mapping) with descrpt(coord_ext, atype_ext,
nl, mapping, comm_dict, fparam) or the correct child-descriptor signature) so
each element of self.descrpt_list receives the frame parameters; keep the
existing logic for nl selection and ensure the passed variables use the same
names (comm_dict and fparam).
In `@deepmd/pt/model/descriptor/se_r.py`:
- Around line 430-473: The forward method currently accepts fparam but doesn't
discard it explicitly; to satisfy linter rules, explicitly discard fparam in the
same way mapping and comm_dict are discarded (e.g., add fparam to the existing
del statement), referencing the forward signature and the existing del mapping,
comm_dict usage so the unused parameter is removed from scope.
In `@deepmd/pt/model/descriptor/se_t.py`:
- Around line 339-391: The forward method currently accepts fparam but never
uses it, triggering an ARG002 lint; inside the forward method of se_t (function
name forward) explicitly discard the unused parameter (for example assign it to
_ or call del fparam) before returning/using other values so Ruff no longer
flags it; ensure this change is placed near the start of forward (before calling
self.seat.forward) so the parameter is visibly consumed and no behavior changes
occur.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2eaaf4c6-3eba-4633-90dc-092ec27c9cd2
📒 Files selected for processing (25)
deepmd/dpmodel/atomic_model/dp_atomic_model.pydeepmd/dpmodel/descriptor/dpa1.pydeepmd/dpmodel/descriptor/dpa2.pydeepmd/dpmodel/descriptor/dpa3.pydeepmd/dpmodel/descriptor/hybrid.pydeepmd/dpmodel/descriptor/make_base_descriptor.pydeepmd/dpmodel/descriptor/se_e2_a.pydeepmd/dpmodel/descriptor/se_r.pydeepmd/dpmodel/descriptor/se_t.pydeepmd/dpmodel/descriptor/se_t_tebd.pydeepmd/pt/model/atomic_model/dp_atomic_model.pydeepmd/pt/model/descriptor/dpa1.pydeepmd/pt/model/descriptor/dpa2.pydeepmd/pt/model/descriptor/dpa3.pydeepmd/pt/model/descriptor/hybrid.pydeepmd/pt/model/descriptor/se_a.pydeepmd/pt/model/descriptor/se_r.pydeepmd/pt/model/descriptor/se_t.pydeepmd/pt/model/descriptor/se_t_tebd.pydeepmd/utils/argcheck.pysource/tests/consistent/descriptor/common.pysource/tests/consistent/descriptor/test_dpa3.pysource/tests/pt/model/test_dpa3.pysource/tests/universal/dpmodel/descriptor/test_descriptor.pysource/tests/universal/dpmodel/model/test_model.py
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| fix_stat_std, | ||
| n_multi_edge_message, | ||
| precision, | ||
| add_chg_spin_ebd, |
Check notice
Code scanning / CodeQL
Unused local variable Note test
| fix_stat_std, | ||
| n_multi_edge_message, | ||
| precision, | ||
| add_chg_spin_ebd, |
Check notice
Code scanning / CodeQL
Unused local variable Note test
| fix_stat_std, | ||
| n_multi_edge_message, | ||
| precision, | ||
| add_chg_spin_ebd, |
Check notice
Code scanning / CodeQL
Unused local variable Note test
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/tests/consistent/descriptor/common.py (1)
233-256:⚠️ Potential issue | 🟠 MajorPass
fparamthrough the Paddle path too.Line 233 adds
fparam, but Lines 251-256 still ignore it. That makes the Paddle helper inconsistent with the other backends and prevents these tests from actually exercising the new frame-parameter path there. It also leaves the new argument unused, which Ruff is already flagging.Proposed fix
def eval_pd_descriptor( self, pd_obj: Any, natoms: np.ndarray, coords: np.ndarray, atype: np.ndarray, box: np.ndarray, mixed_types: bool = False, fparam: np.ndarray | None = None, ) -> Any: ext_coords, ext_atype, mapping = extend_coord_with_ghosts_pd( paddle.to_tensor(coords).to(PD_DEVICE).reshape([1, -1, 3]), paddle.to_tensor(atype).to(PD_DEVICE).reshape([1, -1]), paddle.to_tensor(box).to(PD_DEVICE).reshape([1, 3, 3]), pd_obj.get_rcut(), ) nlist = build_neighbor_list_pd( ext_coords, ext_atype, natoms[0], pd_obj.get_rcut(), pd_obj.get_sel(), distinguish_types=(not mixed_types), ) + fparam_pd = ( + paddle.to_tensor(fparam).to(PD_DEVICE) if fparam is not None else None + ) return [ x.detach().cpu().numpy() if paddle.is_tensor(x) else x for x in pd_obj( ext_coords, ext_atype, nlist=nlist, mapping=mapping, + fparam=fparam_pd, ) ]As per coding guidelines,
Always run ruff check .andruff format .before committing changes or CI will fail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/tests/consistent/descriptor/common.py` around lines 233 - 256, The Paddle path forgets to pass the new fparam argument through to the descriptor call; update the block that prepares inputs for pd_obj to convert fparam to a Paddle tensor (when not None) in the same style as coords/atype/box (e.g. paddle.to_tensor(fparam).to(PD_DEVICE).reshape([...]) or leave None if fparam is None) and include it in the pd_obj(...) call as fparam=... so the Paddle helper actually uses the frame-parameter path (refer to pd_obj(...) and the fparam parameter added at the function signature).
🤖 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/dpmodel/atomic_model/dp_atomic_model.py`:
- Around line 210-216: The unpacking from self.descriptor currently binds an
unused variable `sw` which triggers RUF059; update the unpack assignment to use
a throwaway name (e.g., `_sw` or `_`) instead of `sw` so the unused value is
ignored (change "descriptor, rot_mat, g2, h2, sw = self.descriptor(...)" to
"descriptor, rot_mat, g2, h2, _sw = self.descriptor(...)" or use `_`).
- Around line 187-208: The fallback that materializes a default fparam should
only run when descriptor charge/spin embedding is being used and a true default
exists: change the condition around the default fparam branch in dp_atomic_model
(the block using self.fitting_net.get_dim_fparam(), get_default_fparam(), and
array_api_compat) to also require the descriptor embedding flag
(add_chg_spin_ebd) is true and that the fitter exposes a real default via a
has_default_fparam() check (or equivalent) before calling get_default_fparam();
if has_default_fparam() is not present, treat that as “no default” and keep
fparam_input_for_des = fparam. Ensure you only call get_default_fparam() and
construct default_fparam_array when add_chg_spin_ebd is true and a default is
confirmed.
In `@deepmd/pt/model/atomic_model/dp_atomic_model.py`:
- Around line 292-299: The call to self.descriptor(...) in dp_atomic_model.py
currently assigns five returns to descriptor, rot_mat, g2, h2, sw but sw is
unused; rename this binding to _sw (or _) to satisfy linters. Update the
assignment in the dp_atomic_model.AtomicModel (or the method containing that
descriptor call) to use _sw instead of sw so Ruff no longer flags an unused
variable, and run ruff check . / ruff format . before committing.
- Around line 277-290: The branch that supplies a default fparam must only run
when an actual default exists; update the condition in the block that currently
checks self.fitting_net.get_dim_fparam() > 0 so it also requires
self.fitting_net.get_default_fparam() is not None and fparam is None before
using the default. Concretely, change the if condition that references
get_dim_fparam() to also call get_default_fparam() (or store it first) and only
tile/use default_fparam_tensor when that default is present; otherwise leave
fparam_input_for_des = fparam so forward_atomic()/forward path won’t assert on
None. Ensure you reference the same symbols: self.fitting_net.get_dim_fparam(),
self.fitting_net.get_default_fparam(), and forward_atomic().
---
Outside diff comments:
In `@source/tests/consistent/descriptor/common.py`:
- Around line 233-256: The Paddle path forgets to pass the new fparam argument
through to the descriptor call; update the block that prepares inputs for pd_obj
to convert fparam to a Paddle tensor (when not None) in the same style as
coords/atype/box (e.g. paddle.to_tensor(fparam).to(PD_DEVICE).reshape([...]) or
leave None if fparam is None) and include it in the pd_obj(...) call as
fparam=... so the Paddle helper actually uses the frame-parameter path (refer to
pd_obj(...) and the fparam parameter added at the function signature).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 64878d18-a583-4171-b914-7da518ae434f
📒 Files selected for processing (3)
deepmd/dpmodel/atomic_model/dp_atomic_model.pydeepmd/pt/model/atomic_model/dp_atomic_model.pysource/tests/consistent/descriptor/common.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5295 +/- ##
==========================================
- Coverage 82.32% 82.29% -0.03%
==========================================
Files 768 773 +5
Lines 77098 77415 +317
Branches 3659 3660 +1
==========================================
+ Hits 63469 63712 +243
- Misses 12458 12529 +71
- Partials 1171 1174 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
deepmd/pt/model/atomic_model/dp_atomic_model.py (2)
291-298:⚠️ Potential issue | 🟡 MinorRename the unused
swbinding.
swis still never read after the descriptor call, so Ruff will keep flagging this unpack. Rename it to_sw(or_) before merging. As per coding guidelines,**/*.py: Always runruff check .andruff format .before committing changes or CI will fail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deepmd/pt/model/atomic_model/dp_atomic_model.py` around lines 291 - 298, The tuple returned by self.descriptor is unpacked into descriptor, rot_mat, g2, h2, sw but sw is never used; rename the binding to _sw (or _) in the unpack expression in dp_atomic_model.py (the call to self.descriptor with args extended_coord, extended_atype, nlist, mapping, comm_dict, fparam=...) so the unused value is ignored and Ruff stops flagging the unused variable; run ruff check . and ruff format . before committing.
277-289:⚠️ Potential issue | 🔴 CriticalDon’t assume
get_dim_fparam() > 0implies a default exists.Line 283 still dereferences
get_default_fparam()based only onget_dim_fparam() > 0, but fittings can legally have frame-parameter support with no configured default. In that caseforward_atomic()still hits the assertion when callers omitfparam. Gate this branch on an actual default (or raise a clear error only for the charge/spin path) instead of asserting onNone.🐛 Proposed fix
- if ( - hasattr(self.fitting_net, "get_dim_fparam") - and self.fitting_net.get_dim_fparam() > 0 - and fparam is None - ): - # use default fparam - default_fparam_tensor = self.fitting_net.get_default_fparam() - assert default_fparam_tensor is not None + default_fparam_tensor = self.fitting_net.get_default_fparam() + if self.add_chg_spin_ebd and fparam is None: + if default_fparam_tensor is None: + raise ValueError( + "fparam must be provided when add_chg_spin_ebd is enabled " + "and no default frame parameter is configured" + ) fparam_input_for_des = torch.tile( default_fparam_tensor.unsqueeze(0), [nframes, 1] ) else: fparam_input_for_des = fparam🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deepmd/pt/model/atomic_model/dp_atomic_model.py` around lines 277 - 289, The current branch assumes a default fparam exists when fitting_net.get_dim_fparam() > 0 and then asserts on get_default_fparam(); instead, first query get_default_fparam() and only use it when it is not None: call self.fitting_net.get_default_fparam() and if that returns a tensor build fparam_input_for_des by tiling it (as currently done). If get_default_fparam() is None and fparam is None, raise a clear ValueError (or a specific error) explaining that no fparam was provided and no default is configured (but only for the charge/spin path if that special handling is needed); otherwise set fparam_input_for_des = fparam. Update the logic around fitting_net.get_dim_fparam, get_default_fparam, forward_atomic and fparam_input_for_des to reflect this check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@deepmd/pt/model/atomic_model/dp_atomic_model.py`:
- Around line 291-298: The tuple returned by self.descriptor is unpacked into
descriptor, rot_mat, g2, h2, sw but sw is never used; rename the binding to _sw
(or _) in the unpack expression in dp_atomic_model.py (the call to
self.descriptor with args extended_coord, extended_atype, nlist, mapping,
comm_dict, fparam=...) so the unused value is ignored and Ruff stops flagging
the unused variable; run ruff check . and ruff format . before committing.
- Around line 277-289: The current branch assumes a default fparam exists when
fitting_net.get_dim_fparam() > 0 and then asserts on get_default_fparam();
instead, first query get_default_fparam() and only use it when it is not None:
call self.fitting_net.get_default_fparam() and if that returns a tensor build
fparam_input_for_des by tiling it (as currently done). If get_default_fparam()
is None and fparam is None, raise a clear ValueError (or a specific error)
explaining that no fparam was provided and no default is configured (but only
for the charge/spin path if that special handling is needed); otherwise set
fparam_input_for_des = fparam. Update the logic around
fitting_net.get_dim_fparam, get_default_fparam, forward_atomic and
fparam_input_for_des to reflect this check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bc8c4ba8-09a5-49d0-a716-d1590293c7af
📒 Files selected for processing (1)
deepmd/pt/model/atomic_model/dp_atomic_model.py
wanghan-iapcm
requested changes
Mar 10, 2026
wanghan-iapcm
approved these changes
Mar 12, 2026
njzjz
approved these changes
Mar 12, 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
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