pd: update loc_mapping for dpa3 in paddle backend #4797
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| assert list(atype_embd.shape) == [nframes, nloc, self.n_dim] | ||
| assert isinstance(atype_embd, paddle.Tensor) # for jit | ||
| node_ebd = self.act(atype_embd) | ||
| n_dim = node_ebd.shape[-1] |
Check notice
Code scanning / CodeQL
Unused local variable Note
📝 WalkthroughWalkthroughThis update introduces dynamic neighbor and angle selection in descriptor and repflow layers, adds options for edge initialization with direct distances and exponential switch smoothing, enables local index mapping in descriptor computations, and refactors switching functions. It adds utility functions for aggregation and graph indexing, implements a JIT-compiled SiLUT activation, and updates device assignment and test skip logic. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DescrptDPA3
participant DescrptBlockRepflows
participant RepFlowLayer
participant Utils
User->>DescrptDPA3: forward(input, ...)
DescrptDPA3->>DescrptBlockRepflows: forward(..., use_loc_mapping, use_dynamic_sel, ...)
DescrptBlockRepflows->>Utils: get_graph_index(nlist, nlist_mask, a_nlist_mask, nall) [if use_dynamic_sel]
DescrptBlockRepflows->>RepFlowLayer: forward(..., edge_index, angle_index)
RepFlowLayer->>Utils: aggregate(data, owners, ...)
RepFlowLayer-->>DescrptBlockRepflows: node/edge/angle updates
DescrptBlockRepflows-->>DescrptDPA3: updated descriptors
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (3.3.7)deepmd/pd/model/network/utils.pyNo files to lint: exiting. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
HydrogenSulfate
force-pushed
the
update_dpa3_in_pd_bkd
branch
from
08b4838 to
74a83a7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
deepmd/pd/utils/utils.py (2)
121-121: Missing gradient returns in backward method.The backward method only returns
grad_inputbut the forward method has four inputs (x, threshold, slope, const_val). The backward must return a gradient for each input.
141-141: Incomplete gradient returns in double backward.The backward method returns two gradients but the forward signature has four inputs (
x, grad_output, threshold, slope). The backward must return a tuple with four elements.
🧹 Nitpick comments (9)
deepmd/pd/utils/preprocess.py (2)
13-17: Polynomial weight: rename for readability & avoid extra multiply.The current form is mathematically correct but hard to parse (
uu2 * uu * (…)). A more idiomatic version keeps the classic 5-3 smoothing polynomial explicit and saves one multiply:-uu2 = uu * uu -vv = uu2 * uu * (-6 * uu2 + 15 * uu - 10) + 1 +poly = 6 * uu**5 - 15 * uu**4 + 10 * uu**3 # P(u) ∈ [0,1] +vv = 1 - polySame result, clearer intent, slightly faster (1 fuse multiply-add chain shorter).
20-29: ExposeCand validate domain incompute_exp_sw.
C = 20is a magic number. Make it a keyword argument with a default so callers can tune the sharpness.
Also guard against negativermin.-def compute_exp_sw(distance, rmin: float, rmax: float): +def compute_exp_sw(distance, + rmin: float, + rmax: float, + C: float = 20.0): @@ - C = 20 - a = C / rmin + a = C / rmindeepmd/pd/entrypoints/main.py (1)
44-46: Import-onlyenvfor its side effects → fragile.
from deepmd.pd.utils import envrelies on the import causing module-level code to run.
Prefer an explicitimport deepmd.pd.utils.env as envto avoid accidental shadowing if the package layout changes.deepmd/pd/model/descriptor/dpa3.py (1)
496-496: Remove unused variablenall.The variable
nallis computed but never used in the function.Apply this diff to remove the unused variable:
- nall = extended_coord.reshape([nframes, -1]).shape[1] // 3🧰 Tools
🪛 Ruff (0.11.9)
496-496: Local variable
nallis assigned to but never usedRemove assignment to unused variable
nall(F841)
deepmd/pd/model/descriptor/repflows.py (1)
465-465: Remove unused variablen_dim.The variable
n_dimis assigned but never used in the function.Apply this diff to remove the unused variable:
- n_dim = node_ebd.shape[-1]🧰 Tools
🪛 Ruff (0.11.9)
465-465: Local variable
n_dimis assigned to but never usedRemove assignment to unused variable
n_dim(F841)
🪛 GitHub Check: CodeQL
[notice] 465-465: Unused local variable
Variable n_dim is not used.deepmd/pd/model/descriptor/repflow_layer.py (4)
752-753: Moven_edgecalculation inside dynamic selection block.The
n_edgecalculation is only used in dynamic mode but is computed unconditionally. Consider moving it inside theif self.use_dynamic_sel:block for efficiency.-n_edge = int(nlist_mask.sum().item()) if paddle.in_dynamic_mode(): assert [nb, nloc] == node_ebd.shape[:2] if not self.use_dynamic_sel: if paddle.in_dynamic_mode(): assert [nb, nloc, nnei, 3] == h2.shape else: + n_edge = int(nlist_mask.sum().item()) if paddle.in_dynamic_mode(): assert [n_edge, 3] == h2.shape
959-960: Add comment explaining dimension slicing for angle compression.The slicing logic is correct, but it would be helpful to add a comment explaining why only the first dimensions are used when
a_compress_use_splitis True.else: + # When using split compression, use the first n_a_compress_dim/e_a_compress_dim dimensions + # of node and edge embeddings for angle calculations node_ebd_for_angle = node_ebd[..., : self.n_a_compress_dim] edge_ebd_for_angle = edge_ebd[..., : self.e_a_compress_dim]
1076-1079: Consider providing more context in the error message.The error message could be more informative about why this combination is not supported.
if self.use_dynamic_sel: raise NotImplementedError( - "smooth_edge_update must be True when use_dynamic_sel is True!" + "smooth_edge_update=False is not supported with use_dynamic_sel=True " + "because dynamic selection requires continuous edge updates without masking operations." )
808-809: Consider consistent scale factor notation.The scale factors use different notations:
dynamic_e_sel ** (-0.5)vs division bydynamic_a_sel ** 0.5. Consider using consistent notation for clarity.Either use negative exponents consistently:
scale_factor=self.dynamic_e_sel ** (-0.5) # line 808 ... / (self.dynamic_a_sel ** (-0.5)) # line 1071Or use positive exponents in division consistently:
scale_factor=1.0 / (self.dynamic_e_sel ** 0.5) # line 808 ... / (self.dynamic_a_sel ** 0.5) # line 1071 (already like this)Also applies to: 1071-1071
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
deepmd/pd/entrypoints/main.py(2 hunks)deepmd/pd/model/descriptor/dpa3.py(5 hunks)deepmd/pd/model/descriptor/env_mat.py(6 hunks)deepmd/pd/model/descriptor/repflow_layer.py(17 hunks)deepmd/pd/model/descriptor/repflows.py(10 hunks)deepmd/pd/model/descriptor/repformers.py(2 hunks)deepmd/pd/model/network/utils.py(1 hunks)deepmd/pd/train/training.py(2 hunks)deepmd/pd/utils/env.py(2 hunks)deepmd/pd/utils/preprocess.py(1 hunks)deepmd/pd/utils/spin.py(0 hunks)deepmd/pd/utils/utils.py(3 hunks)
💤 Files with no reviewable changes (1)
- deepmd/pd/utils/spin.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
deepmd/pd/model/descriptor/env_mat.py (1)
deepmd/pd/utils/preprocess.py (2)
compute_exp_sw(20-29)compute_smooth_weight(9-17)
🪛 Ruff (0.11.9)
deepmd/pd/model/descriptor/dpa3.py
496-496: Local variable nall is assigned to but never used
Remove assignment to unused variable nall
(F841)
deepmd/pd/model/descriptor/repflows.py
465-465: Local variable n_dim is assigned to but never used
Remove assignment to unused variable n_dim
(F841)
🪛 GitHub Check: CodeQL
deepmd/pd/model/descriptor/repflows.py
[notice] 465-465: Unused local variable
Variable n_dim is not used.
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Test Python (5, 3.12)
- GitHub Check: Test Python (6, 3.12)
- GitHub Check: Test Python (6, 3.9)
- GitHub Check: Test Python (5, 3.9)
- GitHub Check: Test Python (4, 3.9)
- GitHub Check: Test Python (1, 3.12)
- GitHub Check: Test Python (1, 3.9)
- GitHub Check: Test Python (4, 3.12)
- GitHub Check: Test Python (3, 3.9)
- GitHub Check: Test Python (2, 3.9)
- GitHub Check: Test Python (3, 3.12)
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Test C++ (false)
- GitHub Check: Test C++ (true)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Build C library (2.14, >=2.5.0rc0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (12)
deepmd/pd/utils/env.py (1)
199-203:__all__entry is fine – no action needed.deepmd/pd/model/descriptor/repformers.py (2)
270-274: Device move change looks good.Switching from
.to(device=env.DEVICE)to.to(env.DEVICE)is equivalent in Paddle and removes the redundant kw-arg.
No functional or performance impact.
506-512: Same minor simplification – OK.The new
.to(env.DEVICE)style is consistent with the earlier change.deepmd/pd/model/descriptor/env_mat.py (2)
18-42: LGTM! Well-implemented exponential switch function option.The changes correctly introduce the
use_exp_switchparameter and implement safe coordinate indexing with padding. The conditional weight calculation properly switches between the polynomial and exponential smoothing functions based on the flag.
61-88: Parameter properly propagated with clear documentation.The
use_exp_switchparameter is correctly added to the function signature and passed through to_make_env_mat. The documentation clearly explains the parameter's purpose.deepmd/pd/model/network/utils.py (1)
47-135: Well-implemented graph indexing utility.The function correctly computes index mappings for edge and angle graphs with proper handling of masks, broadcasting, and tensor operations. The implementation efficiently generates the required indices for graph-based computations.
deepmd/pd/model/descriptor/dpa3.py (2)
120-164: Constructor properly updated with new parameters.The default value change for
use_loc_mappingand the addition ofedge_init_use_distanduse_exp_switchparameters are correctly integrated with theDescrptBlockRepflowsinitialization.
492-501: Correct implementation of local mapping optimization.The parallel mode detection and conditional node embedding computation properly optimize for non-parallel scenarios when
use_loc_mappingis enabled, avoiding unnecessary computation for extended atoms.🧰 Tools
🪛 Ruff (0.11.9)
496-496: Local variable
nallis assigned to but never usedRemove assignment to unused variable
nall(F841)
deepmd/pd/model/descriptor/repflows.py (1)
191-240: Well-structured parameter additions with proper validation.The new parameters (
edge_init_use_dist,use_exp_switch,use_dynamic_sel,sel_reduce_factor,use_loc_mapping) are properly integrated with sensible defaults. The validation logic correctly ensures thatsmooth_edge_updatemust be True whenuse_dynamic_selis enabled, and thatsel_reduce_factoris positive.deepmd/pd/model/descriptor/repflow_layer.py (3)
332-386: LGTM! Well-implemented dynamic version of_cal_hg.The method correctly handles flattened tensors and uses the
aggregatefunction for proper reduction based on owner indices. The scale factor application ensures correct normalization.
537-557: Good fix! Correctly handles both edge directions in angle updates.The changes properly split the matrix into 4 components and include both
sub_edge_update_ikandsub_edge_update_ijin the final calculation. This fixes the missing edge component in the original implementation.
1005-1013: Correct axis parameter fixes for concatenation operations.The changes from
axis=1toaxis=-1are important bug fixes that ensure proper concatenation along the feature dimension, especially for dynamic mode with flattened tensors.
HydrogenSulfate
force-pushed
the
update_dpa3_in_pd_bkd
branch
from
74a83a7 to
5103f5d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
deepmd/pd/model/descriptor/repflow_layer.py (1)
662-679: Variable reuse obscures intent and risks shape misuse
sub_node_updateis first a 3-D tensor, reshaped and immediately
re-assigned with the indexed result:sub_node_update = paddle.matmul(node_ebd, node) # nf x nloc x d sub_node_update = paddle.index_select(sub_node_update.reshape(...), n2e_index, 0)Later the same identifier is used only for its
.shape[-1]
when reshapingsub_node_ext_update, but by then it carries the
n_edge-shaped tensor. That works today yet is brittle and
confusing.Consider using distinct names:
- sub_node_update = paddle.matmul(node_ebd, node) - sub_node_update = paddle.index_select(sub_node_update.reshape(...), n2e_index, 0) + node_proj = paddle.matmul(node_ebd, node) # nf x nloc x d + sub_node_update = paddle.index_select(node_proj.reshape(...), n2e_index, 0)This avoids accidental coupling of shapes in future edits.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deepmd/pd/model/descriptor/repflow_layer.py(17 hunks)deepmd/pd/model/network/utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- deepmd/pd/model/network/utils.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
deepmd/pd/model/descriptor/repflow_layer.py (4)
deepmd/pd/model/network/utils.py (1)
aggregate(9-44)deepmd/dpmodel/descriptor/repflows.py (4)
_cal_hg_dynamic(722-774)symmetrization_op_dynamic(777-831)optim_angle_update_dynamic(1127-1184)optim_edge_update_dynamic(1229-1275)deepmd/pd/model/descriptor/repformer_layer.py (1)
_cal_grrg(1000-1024)deepmd/pd/model/descriptor/repflows.py (1)
forward(396-585)
⏰ Context from checks skipped due to timeout of 90000ms (29)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Test Python (6, 3.12)
- GitHub Check: Test Python (5, 3.9)
- GitHub Check: Test Python (3, 3.9)
- GitHub Check: Test Python (5, 3.12)
- GitHub Check: Test Python (6, 3.9)
- GitHub Check: Test Python (4, 3.12)
- GitHub Check: Test Python (4, 3.9)
- GitHub Check: Test Python (2, 3.9)
- GitHub Check: Test Python (3, 3.12)
- GitHub Check: Test Python (1, 3.12)
- GitHub Check: Test Python (1, 3.9)
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Build C++ (cpu, cpu)
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Build C++ (cuda, cuda)
- GitHub Check: Build C library (2.14, >=2.5.0rc0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Test C++ (false)
- GitHub Check: Test C++ (true)
🔇 Additional comments (1)
deepmd/pd/model/descriptor/repflow_layer.py (1)
752-753:.item()inside graph may break static moden_edge = int(nlist_mask.sum().item())Calling
.item()materialises the value on the host and is illegal in
Paddle static-graph / JIT mode. Prefer purely tensor operations or pass
n_edgefrom pre-computed metadata.n_edge = paddle.cast(nlist_mask, 'int64').sum()or simply rely on
edge_index.shape[0]which is already provided.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #4797 +/- ##
==========================================
- Coverage 84.79% 84.57% -0.22%
==========================================
Files 698 699 +1
Lines 67824 68035 +211
Branches 3540 3540
==========================================
+ Hits 57508 57540 +32
- Misses 9181 9361 +180
+ Partials 1135 1134 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deepmd/pd/model/descriptor/dpa3.py (1)
495-497: Remove unused local variablenallto satisfy static analysis and avoid dead code
nallis calculated but never consumed. Linting (Ruff F841) will fail CI.- nframes, nloc, nnei = nlist.shape - nall = extended_coord.reshape([nframes, -1]).shape[1] // 3 + _, nloc, _ = nlist.shapeRemoving the variable keeps
nlocin scope while eliminating the redundant reshape.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deepmd/pd/model/descriptor/dpa3.py(5 hunks)deepmd/pd/utils/env.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- deepmd/pd/utils/env.py
🧰 Additional context used
🪛 Ruff (0.11.9)
deepmd/pd/model/descriptor/dpa3.py
496-496: Local variable nall is assigned to but never used
Remove assignment to unused variable nall
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Test Python (6, 3.12)
- GitHub Check: Test Python (6, 3.9)
- GitHub Check: Test Python (5, 3.9)
- GitHub Check: Test Python (5, 3.12)
- GitHub Check: Test Python (3, 3.12)
- GitHub Check: Test Python (4, 3.12)
- GitHub Check: Test Python (4, 3.9)
- GitHub Check: Test Python (3, 3.9)
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Test Python (2, 3.9)
- GitHub Check: Test Python (1, 3.9)
- GitHub Check: Test Python (1, 3.12)
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Build C library (2.14, >=2.5.0rc0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Test C++ (false)
- GitHub Check: Test C++ (true)
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (3)
deepmd/pd/model/descriptor/dpa3.py (3)
120-120: Defaultinguse_loc_mappingtoTrueis a breaking behavioural change – verify downstream impactPreviously ‟local index mapping” was opt-in; after this change every training / inference run that does not explicitly specify the flag will silently switch to the new code path.
Please confirm that:
- Legacy checkpoints created with
use_loc_mapping=Falsecan still be deserialized and produce numerically identical results, and- External scripts that instantiate
DescrptDPA3without keyword arguments are prepared for the performance / memory implications of the new path.If either cannot be guaranteed, consider keeping the default
Falseand toggling via model-level metadata instead.
163-165: New kwargs forwarded toDescrptBlockRepflows– ensure backward compatibility with storedRepFlowArgs
edge_init_use_distanduse_exp_switchare forwarded unconditionally.
Older checkpoints that were saved with an earlierRepFlowArgsschema may miss these attributes, leading toAttributeErrorat deserialization time.Option A (minimal): provide default attributes inside
RepFlowArgs.__getattr__/__post_init__.
Option B (explicit): guard the access here:edge_init_use_dist=getattr(self.repflow_args, "edge_init_use_dist", False), use_exp_switch=getattr(self.repflow_args, "use_exp_switch", False),Please validate that unit/compatibility tests cover model loading from previous DeepMD-kit versions.
498-502: Potential shape mismatch betweennode_ebd_extandextended_coordwhen local mapping is activeWhen
use_loc_mappingisTrue(now the default) and not in parallel mode, only the firstnlocatoms receive embeddings:node_ebd_ext = self.type_embedding(extended_atype[:, :nloc])Yet the full
extended_coordandextended_atypetensors (sizenall) are still passed toself.repflows.
IfDescrptBlockRepflowsexpectsnode_ebd_ext.shape[1] == extended_coord.shape[1] // 3, it will raise a dimension assertion at runtime.Please run an inference pass with a non-trivial
nneito confirm that:node_ebd_ext.shape == (nf, nall, dim) # expected inside repflows?If alignment is required, consider always embedding the full
extended_atypeand slice only when concatenating:- if not parallel_mode and self.use_loc_mapping: - node_ebd_ext = self.type_embedding(extended_atype[:, :nloc]) + node_ebd_ext = self.type_embedding(extended_atype)and keep
node_ebd_inp = node_ebd_ext[:, :nloc, :]unchanged.
There was a problem hiding this comment.
Pull Request Overview
This PR adapts the Paddle backend to support local index mapping (loc_mapping) for DPA3 and enriches activation and switching options via JIT and exponential functions.
- Added JIT-compiled SiLUT activation (
SiLUTScript) controlled by a newCUSTOM_OP_USE_JITflag. - Introduced an exponential switch function (
compute_exp_sw) anduse_exp_switchoption throughout environment matrix generation and descriptor blocks. - Enabled local indexing (
use_loc_mapping) and dynamic neighbor selection in RepFlows and DPA3 descriptors.
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| deepmd/pd/utils/utils.py | Added SiLUT JIT layer and CUSTOM_OP_USE_JIT toggle |
| deepmd/pd/utils/spin.py | Removed explicit device placement in paddle.zeros |
| deepmd/pd/utils/preprocess.py | Refactored compute_smooth_weight, added compute_exp_sw |
| deepmd/pd/utils/env.py | Introduced CUSTOM_OP_USE_JIT env flag |
| deepmd/pd/model/network/utils.py | New aggregate and get_graph_index utility functions |
| deepmd/pd/model/descriptor/repformers.py | Updated tensor .to(...) usage; minor buffer registration |
| deepmd/pd/model/descriptor/repflows.py | Integrated loc_mapping, dynamic selection, and exp switch |
| deepmd/pd/model/descriptor/env_mat.py | Added use_exp_switch parameter to env matrix builders |
| deepmd/pd/model/descriptor/dpa3.py | Enabled use_loc_mapping by default and updated forward |
Comments suppressed due to low confidence (1)
deepmd/pd/model/descriptor/repflows.py:501
- The call to
get_graph_indexincludes an unexpected keyworduse_loc_mapping, but its signature only accepts (nlist, nlist_mask, a_nlist_mask, nall). Remove or update the argument to match the function definition.
use_loc_mapping=self.use_loc_mapping,
There was a problem hiding this comment.
Please remove the skip options in UTs: https://github.com/deepmodeling/deepmd-kit/blob/devel/source/tests/consistent/descriptor/test_dpa3.py#L177-L180
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
source/tests/consistent/descriptor/test_dpa3.py (1)
158-173: Consider removing unused parameter unpacking.The parameter unpacking is no longer used since the method now simply returns
CommonTest.skip_pd. While not harmful, removing it would improve code cleanliness.@property def skip_pd(self) -> bool: - ( - update_residual_init, - exclude_types, - update_angle, - a_compress_rate, - a_compress_e_rate, - a_compress_use_split, - optim_update, - edge_init_use_dist, - use_exp_switch, - use_dynamic_sel, - use_loc_mapping, - fix_stat_std, - n_multi_edge_message, - precision, - ) = self.param return CommonTest.skip_pd
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
source/tests/consistent/descriptor/test_dpa3.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (29)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Build C library (2.14, >=2.5.0rc0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Build C++ (cuda, cuda)
- GitHub Check: Build C++ (cpu, cpu)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Test C++ (false)
- GitHub Check: Test C++ (true)
- GitHub Check: Test Python (6, 3.9)
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Test Python (3, 3.12)
- GitHub Check: Test Python (5, 3.12)
- GitHub Check: Test Python (6, 3.12)
- GitHub Check: Test Python (2, 3.9)
- GitHub Check: Test Python (5, 3.9)
- GitHub Check: Test Python (1, 3.9)
- GitHub Check: Test Python (4, 3.9)
- GitHub Check: Test Python (4, 3.12)
- GitHub Check: Test Python (3, 3.9)
- GitHub Check: Test Python (1, 3.12)
🔇 Additional comments (1)
source/tests/consistent/descriptor/test_dpa3.py (1)
157-174: LGTM! This change enables loc_mapping support for Paddle backend.The simplification of the
skip_pdmethod by removing explicit feature checks (includinguse_loc_mapping) aligns perfectly with the PR objective of updating loc_mapping for dpa3 in the Paddle backend. This indicates that the Paddle backend now supports these previously unsupported features.
Head branch was pushed to by a user without write access
3bd950f
adapt preparing input spec in paddle backend to support
loc_mappingfor dpa3Accuracy test
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores