feat(pt/dpmodel): add sequential_update for dpa3#5355
feat(pt/dpmodel): add sequential_update for dpa3#5355iProzd wants to merge 6 commits intodeepmodeling:masterfrom
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds a new sequential_update mode for the DPA3 repflow layers, wiring it through configuration/argcheck and implementing the sequential edge/angle/node update order in both the PyTorch and array-api (dpmodel) backends.
Changes:
- Introduce
sequential_updateas a configurable repflow argument (docs + argcheck) and propagate it through DPA3 initialization. - Implement sequential-update execution paths in
RepFlowLayerfor bothdeepmd/ptanddeepmd/dpmodel. - Expand DPA3 tests to exercise
sequential_updateacross consistency and JIT scenarios.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| source/tests/pt/model/test_dpa3.py | Adds sequential_update coverage to PT consistency and JIT tests. |
| source/tests/consistent/descriptor/test_dpa3.py | Adds sequential_update to cross-backend descriptor consistency matrix. |
| deepmd/utils/argcheck.py | Documents and registers sequential_update in DPA3 repflow arg schema. |
| deepmd/pt/model/descriptor/repflows.py | Threads sequential_update into PT repflow block and per-layer construction. |
| deepmd/pt/model/descriptor/repflow_layer.py | Implements _forward_sequential and serialization of sequential_update. |
| deepmd/pt/model/descriptor/dpa3.py | Propagates sequential_update from DPA3 args into PT repflow descriptor block. |
| deepmd/dpmodel/descriptor/repflows.py | Threads sequential_update, implements _call_sequential, and serializes it. |
| deepmd/dpmodel/descriptor/dpa3.py | Adds sequential_update to RepFlowArgs API + docs and propagates into repflow construction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a boolean ChangesSequential RepFlow Update
Sequence Diagram(s)sequenceDiagram
participant Config as Descriptor Config
participant Block as DescrptBlockRepflows
participant Layer as RepFlowLayer
participant Edge as Edge/Angle Compute
participant Node as Node Compute
rect rgba(180, 180, 180, 0.5)
Note over Config,Block: Initialization & wiring
Config->>Block: pass sequential_update flag
Block->>Layer: construct layers with sequential_update
end
rect rgba(100, 150, 200, 0.5)
Note over Layer,Edge: Parallel path (sequential_update = False)
Layer->>Edge: compute edge_self, angle_self, edge_angle (parallel phases)
Edge->>Node: aggregated edge/angle data
Node-->>Layer: compute node updates
end
rect rgba(150, 200, 100, 0.5)
Note over Layer,Edge: Sequential path (sequential_update = True)
Layer->>Edge: 1) edge_self -> apply residual -> e'
Layer->>Edge: 2) angle_self using e' -> apply residual -> a'
Layer->>Edge: 3) edge-angle using e', a' -> reduce -> apply residual -> e''
Edge->>Node: 4) node updates using e''
Node-->>Layer: return updated embeddings
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
deepmd/utils/argcheck.py (1)
1553-1559: Mirror the fullsequential_updateconstraint in the schema layer.These docs only mention the
update_style='res_residual'restriction, andargcheckwill still acceptsequential_update=truewithupdate_angle=falseeven thoughRepFlowArgsrejects it later. Anextra_checkon the enclosingrepflowargument would keep config validation and generated docs aligned with runtime behavior.Also applies to: 1690-1696
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deepmd/utils/argcheck.py` around lines 1553 - 1559, Update the repflow argument schema to mirror the runtime constraint described in doc_sequential_update by adding an extra_check that forbids sequential_update=True unless update_style=='res_residual' AND update_angle==True (matching RepFlowArgs). Specifically, modify the repflow schema's extra_check to reference the sequential_update, update_style, and update_angle fields and raise a validation error if sequential_update is true while either update_style != 'res_residual' or update_angle is false, so the schema-level validation and generated docs align with RepFlowArgs runtime checks.source/tests/consistent/descriptor/test_dpa3.py (1)
166-167: Consider prefixing unused unpacked variable with underscore.The static analysis tool (Ruff RUF059) flags
sequential_updateas unpacked but never used in this property. While this follows the existing pattern of unpacking all parameters for consistency, you could prefix it with_to satisfy the linter:- sequential_update, + _sequential_update,This same pattern appears in
skip_pd,skip_dp,skip_tf,setUp,rtol, andatolproperties (lines 188, 210, 232, 298, 405, 433).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/tests/consistent/descriptor/test_dpa3.py` around lines 166 - 167, The unpacked parameter name sequential_update in the property (and similarly in skip_pd, skip_dp, skip_tf, setUp, rtol, atol) is unused and triggers Ruff RUF059; rename it to _sequential_update (and prefix the corresponding unused variables in the listed properties with an underscore) to indicate intentional unused unpacking while preserving the existing tuple unpack pattern and tests.
🤖 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`:
- Line 285: The serialized payload for repflow_args now includes a new key
"sequential_update" but DescrptDPA3.serialize() still emits "@version: 2",
causing old readers to attempt RepFlowArgs(**data) and crash; update
DescrptDPA3.serialize() to bump the serialization version (e.g., to 3) so that
readers can detect the new format, and ensure the version constant/metadata that
serialize() returns is updated accordingly (refer to DescrptDPA3.serialize and
the repflow_args payload construction to locate the change).
In `@deepmd/dpmodel/descriptor/repflows.py`:
- Around line 1602-1622: The non-smooth branch pads masked/out-of-a_sel slots
using edge_ebd, which reverts those slots to the post-update edge state and
breaks the intended sequential ordering; change the padding in the xp.where that
sets padding_edge_angle_update so it uses the phase-1 edge state (the edge
embedding/state used before the angle-stage update) instead of edge_ebd, keeping
the full_mask logic and all other flow (respect
smooth_edge_update/use_dynamic_sel) intact.
---
Nitpick comments:
In `@deepmd/utils/argcheck.py`:
- Around line 1553-1559: Update the repflow argument schema to mirror the
runtime constraint described in doc_sequential_update by adding an extra_check
that forbids sequential_update=True unless update_style=='res_residual' AND
update_angle==True (matching RepFlowArgs). Specifically, modify the repflow
schema's extra_check to reference the sequential_update, update_style, and
update_angle fields and raise a validation error if sequential_update is true
while either update_style != 'res_residual' or update_angle is false, so the
schema-level validation and generated docs align with RepFlowArgs runtime
checks.
In `@source/tests/consistent/descriptor/test_dpa3.py`:
- Around line 166-167: The unpacked parameter name sequential_update in the
property (and similarly in skip_pd, skip_dp, skip_tf, setUp, rtol, atol) is
unused and triggers Ruff RUF059; rename it to _sequential_update (and prefix the
corresponding unused variables in the listed properties with an underscore) to
indicate intentional unused unpacking while preserving the existing tuple unpack
pattern and tests.
🪄 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: 51893b84-b7f2-45e4-b52f-ff66721d1ab0
📒 Files selected for processing (8)
deepmd/dpmodel/descriptor/dpa3.pydeepmd/dpmodel/descriptor/repflows.pydeepmd/pt/model/descriptor/dpa3.pydeepmd/pt/model/descriptor/repflow_layer.pydeepmd/pt/model/descriptor/repflows.pydeepmd/utils/argcheck.pysource/tests/consistent/descriptor/test_dpa3.pysource/tests/pt/model/test_dpa3.py
| "use_exp_switch": self.use_exp_switch, | ||
| "use_dynamic_sel": self.use_dynamic_sel, | ||
| "sel_reduce_factor": self.sel_reduce_factor, | ||
| "sequential_update": self.sequential_update, |
There was a problem hiding this comment.
Bump the DPA3 serialization version for the new repflow_args field.
Line 285 adds a new key to the persisted repflow_args payload, but DescrptDPA3.serialize() still advertises @version: 2. A pre-change v2 reader will accept the file and then die on RepFlowArgs(**data) with an unexpected sequential_update kwarg instead of rejecting the newer format cleanly.
Suggested fix
- "@version": 2,
+ "@version": 3,- check_version_compatibility(version, 2, 1)
+ check_version_compatibility(version, 3, 1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deepmd/dpmodel/descriptor/dpa3.py` at line 285, The serialized payload for
repflow_args now includes a new key "sequential_update" but
DescrptDPA3.serialize() still emits "@version: 2", causing old readers to
attempt RepFlowArgs(**data) and crash; update DescrptDPA3.serialize() to bump
the serialization version (e.g., to 3) so that readers can detect the new
format, and ensure the version constant/metadata that serialize() returns is
updated accordingly (refer to DescrptDPA3.serialize and the repflow_args payload
construction to locate the change).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5355 +/- ##
==========================================
+ Coverage 82.47% 82.49% +0.01%
==========================================
Files 825 825
Lines 87720 87808 +88
Branches 4206 4206
==========================================
+ Hits 72345 72433 +88
Misses 14092 14092
Partials 1283 1283 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
1. Serialization version not bumped (Bug)
sequential_update is added to serialize() in both dpmodel and PT RepFlowLayer, but @version remains at 2 and deserialize() has no backward compat for the missing field. Loading a
model serialized without sequential_update will raise KeyError. Per project convention, this must be done in the same commit:
- Bump
@versionto 3 - Update
check_version_compatibility(data.pop("@version"), 3, 1) - Add
data.setdefault("sequential_update", False)indeserialize()for versions < 3
This applies to both RepFlowLayer and DescrptRepflows in both dpmodel and PT backends.
2. Massive code duplication (~800 lines)
_call_sequential (dpmodel, ~410 lines) and _forward_sequential (PT, ~375 lines) are nearly identical copies of the existing call/forward methods with different residual application
order. Most of the code (edge_info construction, angle compression, node_sym, node_edge, etc.) is identical. This will be a maintenance burden — any future change to the update logic must
be applied in 4 places (call, forward, _call_sequential, _forward_sequential).
Consider refactoring to share code — e.g., factor out the individual phase computations (edge_self_update, angle_self_update, edge_angle_update, node_update) as methods, then have the
sequential vs parallel paths just call them in different order with different inputs.
3. Consistency test doubles parameter combinations
Adding (False, True) for sequential_update to the consistency test test_dpa3.py doubles all DPA3 test cases. Since sequential_update=True only works with update_angle=True, and the test already parametrizes update_angle with (True,) only, this is fine functionally — but it significantly increases CI time for a feature that doesn't affect cross-backend serialization format (the weights are the same, just applied differently).
Signed-off-by: Duo <50307526+iProzd@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
deepmd/dpmodel/descriptor/repflows.py (1)
233-234: ⚡ Quick winDocument
sequential_updatein the public descriptor docstring.The constructor now exposes a new user-facing flag, but the parameter list above still omits it. That makes the new mode easy to miss in generated docs and API help.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deepmd/dpmodel/descriptor/repflows.py` around lines 233 - 234, Add documentation for the new constructor parameter sequential_update to the public descriptor docstring so it appears in generated docs and API help: locate the descriptor's class docstring (the public descriptor for this module that documents the constructor parameters where use_loc_mapping is listed) and add a short sentence describing sequential_update (type: bool, default: False) explaining what enabling sequential update changes in behavior and any important caveats; keep the style consistent with the existing parameter entries and include the default value.source/tests/consistent/descriptor/test_dpa3.py (1)
66-83: ⚡ Quick winPlease keep one uncompressed-angle case in this matrix.
The new sequential path runs through
_prepare_angle_embeddings()and_compute_angle_update()for both compressed and uncompressed angle features, but this sweep now only coversa_compress_rate=1/a_compress_e_rate=2. That drops coverage of the defaulta_compress_rate=0path entirely. If runtime is the concern, I'd keep the broad sweep here and add one small targeted(0, 1)case forsequential_update=True/False.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/tests/consistent/descriptor/test_dpa3.py` around lines 66 - 83, The test matrix removed coverage for the uncompressed-angle path; update the parameterized arguments so at least one test includes a_compress_rate=0 with a_compress_e_rate=1 (to exercise the default uncompressed branch used by _prepare_angle_embeddings() and _compute_angle_update()) while keeping the existing broad sweep for performance; specifically add a small tuple pair (0,1) for (a_compress_rate, a_compress_e_rate) (ensuring sequential_update still iterates True/False) so the sequential_update=True/False paths hit both compressed and uncompressed angle code paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@deepmd/dpmodel/descriptor/repflows.py`:
- Around line 233-234: Add documentation for the new constructor parameter
sequential_update to the public descriptor docstring so it appears in generated
docs and API help: locate the descriptor's class docstring (the public
descriptor for this module that documents the constructor parameters where
use_loc_mapping is listed) and add a short sentence describing sequential_update
(type: bool, default: False) explaining what enabling sequential update changes
in behavior and any important caveats; keep the style consistent with the
existing parameter entries and include the default value.
In `@source/tests/consistent/descriptor/test_dpa3.py`:
- Around line 66-83: The test matrix removed coverage for the uncompressed-angle
path; update the parameterized arguments so at least one test includes
a_compress_rate=0 with a_compress_e_rate=1 (to exercise the default uncompressed
branch used by _prepare_angle_embeddings() and _compute_angle_update()) while
keeping the existing broad sweep for performance; specifically add a small tuple
pair (0,1) for (a_compress_rate, a_compress_e_rate) (ensuring sequential_update
still iterates True/False) so the sequential_update=True/False paths hit both
compressed and uncompressed angle code paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0da0365a-ecbd-4f92-8734-0771e65ed434
📒 Files selected for processing (5)
deepmd/dpmodel/descriptor/dpa3.pydeepmd/dpmodel/descriptor/repflows.pydeepmd/pt/model/descriptor/repflow_layer.pydeepmd/utils/argcheck.pysource/tests/consistent/descriptor/test_dpa3.py
🚧 Files skipped from review as they are similar to previous changes (2)
- deepmd/dpmodel/descriptor/dpa3.py
- deepmd/utils/argcheck.py
|
|
||
| def call( | ||
| def _compute_edge_self_update( | ||
| self, | ||
| node_ebd_ext: Array, # nf x nall x n_dim | ||
| edge_ebd: Array, # nf x nloc x nnei x e_dim | ||
| h2: Array, # nf x nloc x nnei x 3 | ||
| angle_ebd: Array, # nf x nloc x a_nnei x a_nnei x a_dim | ||
| nlist: Array, # nf x nloc x nnei | ||
| nlist_mask: Array, # nf x nloc x nnei | ||
| sw: Array, # switch func, nf x nloc x nnei | ||
| a_nlist: Array, # nf x nloc x a_nnei | ||
| a_nlist_mask: Array, # nf x nloc x a_nnei | ||
| a_sw: Array, # switch func, nf x nloc x a_nnei | ||
| edge_index: Array, # 2 x n_edge | ||
| angle_index: Array, # 3 x n_angle | ||
| ) -> tuple[Array, Array, Array]: | ||
| """ | ||
| Parameters | ||
| ---------- | ||
| node_ebd_ext : nf x nall x n_dim | ||
| Extended node embedding. | ||
| edge_ebd : nf x nloc x nnei x e_dim | ||
| Edge embedding. | ||
| h2 : nf x nloc x nnei x 3 | ||
| Pair-atom channel, equivariant. | ||
| angle_ebd : nf x nloc x a_nnei x a_nnei x a_dim | ||
| Angle embedding. | ||
| nlist : nf x nloc x nnei | ||
| Neighbor list. (padded neis are set to 0) | ||
| nlist_mask : nf x nloc x nnei | ||
| Masks of the neighbor list. real nei 1 otherwise 0 | ||
| sw : nf x nloc x nnei | ||
| Switch function. | ||
| a_nlist : nf x nloc x a_nnei | ||
| Neighbor list for angle. (padded neis are set to 0) | ||
| a_nlist_mask : nf x nloc x a_nnei | ||
| Masks of the neighbor list for angle. real nei 1 otherwise 0 | ||
| a_sw : nf x nloc x a_nnei | ||
| Switch function for angle. | ||
| edge_index : Optional for dynamic sel, 2 x n_edge | ||
| n2e_index : n_edge | ||
| Broadcast indices from node(i) to edge(ij), or reduction indices from edge(ij) to node(i). | ||
| n_ext2e_index : n_edge | ||
| Broadcast indices from extended node(j) to edge(ij). | ||
| angle_index : Optional for dynamic sel, 3 x n_angle | ||
| n2a_index : n_angle | ||
| Broadcast indices from extended node(j) to angle(ijk). | ||
| eij2a_index : n_angle | ||
| Broadcast indices from extended edge(ij) to angle(ijk), or reduction indices from angle(ijk) to edge(ij). | ||
| eik2a_index : n_angle | ||
| Broadcast indices from extended edge(ik) to angle(ijk). | ||
|
|
||
| Returns | ||
| ------- | ||
| n_updated: nf x nloc x n_dim | ||
| Updated node embedding. | ||
| e_updated: nf x nloc x nnei x e_dim | ||
| Updated edge embedding. | ||
| a_updated : nf x nloc x a_nnei x a_nnei x a_dim | ||
| Updated angle embedding. | ||
| """ | ||
| xp = array_api_compat.array_namespace( | ||
| node_ebd_ext, | ||
| edge_ebd, | ||
| h2, | ||
| angle_ebd, | ||
| nlist, | ||
| nlist_mask, | ||
| sw, | ||
| a_nlist, | ||
| a_nlist_mask, | ||
| a_sw, | ||
| edge_index, | ||
| angle_index, | ||
| ) | ||
| nb, nloc, nnei = nlist.shape | ||
| nall = node_ebd_ext.shape[1] | ||
| # int cannot jit; do not run it when self.use_dynamic_sel == False | ||
| n_edge = ( | ||
| int(xp.sum(xp.astype(nlist_mask, xp.int32))) if self.use_dynamic_sel else 0 | ||
| ) | ||
| node_ebd = xp_take_first_n(node_ebd_ext, 1, nloc) | ||
| assert (nb, nloc) == node_ebd.shape[:2] | ||
| if not self.use_dynamic_sel: | ||
| assert (nb, nloc, nnei) == h2.shape[:3] | ||
| else: | ||
| assert (n_edge, 3) == h2.shape | ||
| del a_nlist # may be used in the future | ||
|
|
||
| n2e_index, n_ext2e_index = edge_index[0, :], edge_index[1, :] | ||
| n2a_index, eij2a_index, eik2a_index = ( | ||
| angle_index[0, :], | ||
| angle_index[1, :], | ||
| angle_index[2, :], | ||
| ) | ||
|
|
||
| # nb x nloc x nnei x n_dim [OR] n_edge x n_dim | ||
| nei_node_ebd = ( | ||
| _make_nei_g1(node_ebd_ext, nlist) | ||
| if not self.use_dynamic_sel | ||
| else xp.take( | ||
| xp.reshape(node_ebd_ext, (-1, self.n_dim)), n_ext2e_index, axis=0 | ||
| ) | ||
| ) | ||
|
|
||
| n_update_list: list[Array] = [node_ebd] | ||
| e_update_list: list[Array] = [edge_ebd] | ||
| a_update_list: list[Array] = [angle_ebd] | ||
|
|
||
| # node self mlp | ||
| node_self_mlp = self.act(self.node_self_mlp(node_ebd)) | ||
| n_update_list.append(node_self_mlp) | ||
|
|
||
| # node sym (grrg + drrd) | ||
| node_sym_list: list[Array] = [] | ||
| node_sym_list.append( | ||
| symmetrization_op( | ||
| edge_ebd, | ||
| h2, | ||
| nlist_mask, | ||
| sw, | ||
| self.axis_neuron, | ||
| ) | ||
| if not self.use_dynamic_sel | ||
| else symmetrization_op_dynamic( | ||
| edge_ebd, | ||
| h2, | ||
| sw, | ||
| owner=n2e_index, | ||
| num_owner=nb * nloc, | ||
| nb=nb, | ||
| nloc=nloc, | ||
| scale_factor=self.dynamic_e_sel ** (-0.5), | ||
| axis_neuron=self.axis_neuron, | ||
| ) | ||
| ) | ||
| node_sym_list.append( | ||
| symmetrization_op( | ||
| nei_node_ebd, | ||
| h2, | ||
| nlist_mask, | ||
| sw, | ||
| self.axis_neuron, | ||
| ) | ||
| if not self.use_dynamic_sel | ||
| else symmetrization_op_dynamic( | ||
| nei_node_ebd, | ||
| h2, | ||
| sw, | ||
| owner=n2e_index, | ||
| num_owner=nb * nloc, | ||
| nb=nb, | ||
| nloc=nloc, | ||
| scale_factor=self.dynamic_e_sel ** (-0.5), | ||
| axis_neuron=self.axis_neuron, | ||
| ) | ||
| ) | ||
| node_sym = self.act(self.node_sym_linear(xp.concat(node_sym_list, axis=-1))) | ||
| n_update_list.append(node_sym) | ||
|
|
||
| xp: object, | ||
| node_ebd: Array, | ||
| node_ebd_ext: Array, | ||
| edge_ebd: Array, | ||
| nei_node_ebd: Array, | ||
| nlist: Array, | ||
| n2e_index: Array, | ||
| n_ext2e_index: Array, | ||
| nb: int, | ||
| nloc: int, | ||
| ) -> Array: | ||
| """Compute edge self update.""" | ||
| if not self.optim_update: |
Check warning
Code scanning / CodeQL
Signature mismatch in overriding method Warning
| assert self.a_compress_e_linear is not None | ||
| node_ebd_for_angle = self.a_compress_n_linear(node_ebd) | ||
| edge_ebd_for_angle = self.a_compress_e_linear(edge_ebd) | ||
| else: |
Check notice
Code scanning / CodeQL
Unused local variable Note
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/tests/consistent/descriptor/test_dpa3.py`:
- Around line 208-209: The test unpacks self.param into 17 variables including
sequential_update but the case generator still builds tuples from
DPA3_CASE_FIELDS (16 fields), causing a ValueError; update the case schema to
include the new sequential_update field (or remove sequential_update from
unpacking if it was added accidentally) so all tuple producers and consumers
agree. Modify the case-building list/tuple (the constant DPA3_CASE_FIELDS or the
function that yields test cases) to append the new sequential_update value
across all generated cases, and ensure every unpack site that references
self.param (the test methods that currently unpack into sequential_update)
matches the final tuple arity. Verify by running the tests that no ValueError
arises from tuple unpacking.
🪄 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: 2c799ae1-a437-4eb7-bcb7-55a4eb9e9c98
📒 Files selected for processing (3)
deepmd/pt/model/descriptor/repflows.pydeepmd/utils/argcheck.pysource/tests/consistent/descriptor/test_dpa3.py
🚧 Files skipped from review as they are similar to previous changes (2)
- deepmd/utils/argcheck.py
- deepmd/pt/model/descriptor/repflows.py
| _n_multi_edge_message, | ||
| _precision, | ||
| _add_chg_spin_ebd, | ||
| _sequential_update, |
Check notice
Code scanning / CodeQL
Unused local variable Note test
| _n_multi_edge_message, | ||
| _precision, | ||
| _add_chg_spin_ebd, | ||
| _sequential_update, |
Check notice
Code scanning / CodeQL
Unused local variable Note test
| _n_multi_edge_message, | ||
| _precision, | ||
| _add_chg_spin_ebd, | ||
| _sequential_update, |
Check notice
Code scanning / CodeQL
Unused local variable Note test
There was a problem hiding this comment.
♻️ Duplicate comments (1)
source/tests/consistent/descriptor/test_dpa3.py (1)
282-302:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix tuple arity mismatch in
self.paramunpacking (will break tests).Line 299 and Line 410 introduce a second
sequential_updatetarget in each unpack.self.paramhas 16 fields, but these sites unpack 17 variables, which will fail at runtime.Suggested fix
@@ def skip_pd(self) -> bool: ( @@ _n_multi_edge_message, _precision, add_chg_spin_ebd, - sequential_update, _sequential_update, ) = self.param return True if add_chg_spin_ebd else CommonTest.skip_pd @@ ( @@ _n_multi_edge_message, _precision, add_chg_spin_ebd, - sequential_update, _sequential_update, ) = self.paramAlso applies to: 394-412
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/tests/consistent/descriptor/test_dpa3.py` around lines 282 - 302, The tuple unpack in skip_pd currently unpacks 17 names but self.param has 16 elements, causing a mismatch; in the skip_pd method (and the similar unpack around lines ~394-412) remove the duplicate name (the second sequential_update) from the left-hand side so the number of variables matches self.param, or alternatively replace the duplicated identifier with a single variable name (e.g., keep sequential_update only once) to restore correct arity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@source/tests/consistent/descriptor/test_dpa3.py`:
- Around line 282-302: The tuple unpack in skip_pd currently unpacks 17 names
but self.param has 16 elements, causing a mismatch; in the skip_pd method (and
the similar unpack around lines ~394-412) remove the duplicate name (the second
sequential_update) from the left-hand side so the number of variables matches
self.param, or alternatively replace the duplicated identifier with a single
variable name (e.g., keep sequential_update only once) to restore correct arity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 54dfffdf-2292-4bdf-a375-4ff18c4e2164
📒 Files selected for processing (1)
source/tests/consistent/descriptor/test_dpa3.py
Summary by CodeRabbit