feat(pt/dp/jax/xp): add DPA3 descriptor #4609
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
📝 WalkthroughWalkthroughThe pull request introduces a new descriptor called Changes
Sequence Diagram(s)sequenceDiagram
participant U as User/Input
participant D as DescrptDPA3 Descriptor
participant R as RepFlowLayer/Repflows
participant S as Serialization Engine
U->>D: Call with extended coordinates, atom types, and nlist
D->>R: Process message passing (node, edge, angle embeddings)
R-->>D: Return updated embeddings
D->>S: Serialize state / stats if needed
S-->>D: Return serialized data
D->>U: Return computed descriptor outputs (embeddings, rotations, stats)
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (13)
deepmd/jax/descriptor/repflows.py (1)
23-41: Consider clarifying the rationale behind double serialization.In the
__setattr__method ofDescrptBlockRepflows, line 31 re-serializes each layer withlayer.serialize()before deserializing again. While this may be intentional to ensure proper object structure, adding a short comment would help future maintainers understand the necessity of this procedure.deepmd/pt/model/descriptor/dpa3.py (3)
347-347: Rename unused loop variable for clarity.The loop variable
iiis never used. To clarify intent, rename it to_or_i.- for ii, descrpt in enumerate(descrpt_list): + for _, descrpt in enumerate(descrpt_list):🧰 Tools
🪛 Ruff (0.8.2)
347-347: Loop control variable
iinot used within loop bodyRename unused
iito_ii(B007)
423-423: Remove unused variable "env_mat".The variable
env_matis assigned but never used, which can cause confusion and clutter.- env_mat = repflow_variable.pop("env_mat")🧰 Tools
🪛 Ruff (0.8.2)
423-423: Local variable
env_matis assigned to but never usedRemove assignment to unused variable
env_mat(F841)
🪛 GitHub Check: CodeQL
[notice] 423-423: Unused local variable
Variable env_mat is not used.
479-479: Remove unused assignment to "nall".The local variable
nallis never referenced; removing it can improve clarity.- nall = extended_coord.view(nframes, -1).shape[1] // 3🧰 Tools
🪛 Ruff (0.8.2)
479-479: Local variable
nallis assigned to but never usedRemove assignment to unused variable
nall(F841)
🪛 GitHub Check: CodeQL
[notice] 479-479: Unused local variable
Variable nall is not used.deepmd/pt/model/descriptor/repflows.py (2)
460-460: Rename unused loop variable "idx".Renaming
idxto_idxclarifies that the variable is not used within the loop.- for idx, ll in enumerate(self.layers): + for _idx, ll in enumerate(self.layers):🧰 Tools
🪛 Ruff (0.8.2)
460-460: Loop control variable
idxnot used within loop bodyRename unused
idxto_idx(B007)
570-574: Consider using a ternary operator for conciseness.Replacing the multi-line
if-elseblock with a ternary operator can make the code more concise without loss of clarity.- if callable(merged): - sampled = merged() - else: - sampled = merged + sampled = merged() if callable(merged) else merged🧰 Tools
🪛 Ruff (0.8.2)
570-574: Use ternary operator
sampled = merged() if callable(merged) else mergedinstead ofif-else-block(SIM108)
deepmd/dpmodel/descriptor/repflows.py (1)
436-436: Rename unused loop variable to underscore.The variable
idxis not referenced in the loop. Renaming it to_or_idxclarifies its unused status.-for idx, ll in enumerate(self.layers): +for _idx, ll in enumerate(self.layers):🧰 Tools
🪛 Ruff (0.8.2)
436-436: Loop control variable
idxnot used within loop bodyRename unused
idxto_idx(B007)
source/tests/array_api_strict/descriptor/dpa3.py (1)
22-31: LGTM! Consider adding docstring for clarity.The implementation correctly handles attribute assignments with appropriate type conversions and deserialization. The class registration and inheritance are well-structured.
Consider adding a docstring to explain the purpose of the class and its attribute handling:
@BaseDescriptor.register("dpa3") class DescrptDPA3(DescrptDPA3DP): + """Array API Strict implementation of DPA3 descriptor. + + Handles attribute assignments with appropriate type conversions: + - mean, stddev: Converted to array API strict arrays + - repflows: Deserialized using DescrptBlockRepflows + - type_embedding: Deserialized using TypeEmbedNet + """ def __setattr__(self, name: str, value: Any) -> None:deepmd/jax/descriptor/dpa3.py (1)
23-35: LGTM! Consider adding docstring and return type hint.The implementation correctly handles attribute assignments with appropriate type conversions and deserialization. The class registration, inheritance, and decorators are well-structured.
Consider these improvements:
- Add a docstring to explain the purpose of the class and its attribute handling:
@BaseDescriptor.register("dpa3") @flax_module class DescrptDPA3(DescrptDPA3DP): + """JAX implementation of DPA3 descriptor. + + Handles attribute assignments with appropriate type conversions: + - mean, stddev: Converted to JAX arrays and wrapped in ArrayAPIVariable + - repflows: Deserialized using DescrptBlockRepflows + - type_embedding: Deserialized using TypeEmbedNet + """ def __setattr__(self, name: str, value: Any) -> None:
- Add return type hint for
__setattr__:- def __setattr__(self, name: str, value: Any) -> None: + def __setattr__(self, name: str, value: Any) -> None | Any:source/tests/consistent/descriptor/test_dpa3.py (2)
154-155: Remove commented code and simplify skip condition.The commented code suggests that the PD backend skip condition should be based on installation status and precision, but it's currently hardcoded to
True. Either implement the commented logic or simplify the code.- # return not INSTALLED_PD or precision == "bfloat16" - return True + return not INSTALLED_PD or precision == "bfloat16"
127-136: Remove unused parameters in skip_pt method.The method extracts parameters from
self.parambut doesn't use them. Consider removing the unused parameters to improve code clarity.- ( - update_residual_init, - exclude_types, - update_angle, - a_compress_rate, - a_compress_e_rate, - a_compress_use_split, - optim_update, - fix_stat_std, - n_multi_edge_message, - precision, - ) = self.param return CommonTest.skip_pt🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 127-127: Unused local variable
Variable update_residual_init is not used.
[notice] 128-128: Unused local variable
Variable exclude_types is not used.
[notice] 129-129: Unused local variable
Variable update_angle is not used.
[notice] 130-130: Unused local variable
Variable a_compress_rate is not used.
[notice] 131-131: Unused local variable
Variable a_compress_e_rate is not used.
[notice] 132-132: Unused local variable
Variable a_compress_use_split is not used.
[notice] 133-133: Unused local variable
Variable optim_update is not used.
[notice] 134-134: Unused local variable
Variable fix_stat_std is not used.
[notice] 135-135: Unused local variable
Variable n_multi_edge_message is not used.
[notice] 136-136: Unused local variable
Variable precision is not used.source/tests/universal/pt/model/test_model.py (1)
92-92: Fix typo in variable name.The variable name
defalut_des_paramcontains a typo and should bedefault_des_param.-defalut_des_param = [ +default_des_param = [deepmd/utils/argcheck.py (1)
1426-1587: LGTM! Comprehensive repflow argument configuration.The repflow block configuration is well-defined with:
- Clear parameter documentation
- Appropriate default values
- Proper type validation
- Sensible parameter grouping
Consider adding parameter range constraints in documentation.
For numeric parameters like dimensions and rates, it would be helpful to document any valid ranges or constraints in the docstrings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
deepmd/dpmodel/descriptor/__init__.py(2 hunks)deepmd/dpmodel/descriptor/dpa3.py(1 hunks)deepmd/dpmodel/descriptor/repflows.py(1 hunks)deepmd/dpmodel/utils/network.py(1 hunks)deepmd/jax/descriptor/__init__.py(2 hunks)deepmd/jax/descriptor/dpa3.py(1 hunks)deepmd/jax/descriptor/repflows.py(1 hunks)deepmd/pt/model/descriptor/__init__.py(2 hunks)deepmd/pt/model/descriptor/dpa3.py(1 hunks)deepmd/pt/model/descriptor/repflow_layer.py(1 hunks)deepmd/pt/model/descriptor/repflows.py(1 hunks)deepmd/pt/utils/utils.py(1 hunks)deepmd/utils/argcheck.py(1 hunks)source/tests/array_api_strict/descriptor/dpa3.py(1 hunks)source/tests/array_api_strict/descriptor/repflows.py(1 hunks)source/tests/consistent/descriptor/test_dpa3.py(1 hunks)source/tests/pt/model/test_dpa3.py(1 hunks)source/tests/universal/dpmodel/descriptor/test_descriptor.py(4 hunks)source/tests/universal/pt/descriptor/test_descriptor.py(3 hunks)source/tests/universal/pt/model/test_model.py(15 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
source/tests/consistent/descriptor/test_dpa3.py
39-43: Use ternary operator DescrptDPA3PD = None if INSTALLED_PD else None instead of if-else-block
(SIM108)
source/tests/pt/model/test_dpa3.py
209-209: Local variable model is assigned to but never used
Remove assignment to unused variable model
(F841)
source/tests/universal/dpmodel/descriptor/test_descriptor.py
474-474: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
deepmd/pt/model/descriptor/repflow_layer.py
307-307: Local variable e_dim is assigned to but never used
Remove assignment to unused variable e_dim
(F841)
534-534: Local variable nall is assigned to but never used
Remove assignment to unused variable nall
(F841)
788-788: Local variable nitem is assigned to but never used
Remove assignment to unused variable nitem
(F841)
deepmd/dpmodel/descriptor/dpa3.py
222-222: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
587-587: Local variable env_mat is assigned to but never used
Remove assignment to unused variable env_mat
(F841)
deepmd/pt/model/descriptor/repflows.py
99-99: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
359-359: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
460-460: Loop control variable idx not used within loop body
Rename unused idx to _idx
(B007)
570-574: Use ternary operator sampled = merged() if callable(merged) else merged instead of if-else-block
(SIM108)
deepmd/dpmodel/descriptor/repflows.py
77-77: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
352-352: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
436-436: Loop control variable idx not used within loop body
Rename unused idx to _idx
(B007)
863-863: Local variable nall is assigned to but never used
Remove assignment to unused variable nall
(F841)
1125-1125: Local variable nitem is assigned to but never used
Remove assignment to unused variable nitem
(F841)
deepmd/pt/model/descriptor/dpa3.py
75-75: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
347-347: Loop control variable ii not used within loop body
Rename unused ii to _ii
(B007)
423-423: Local variable env_mat is assigned to but never used
Remove assignment to unused variable env_mat
(F841)
479-479: Local variable nall is assigned to but never used
Remove assignment to unused variable nall
(F841)
🪛 GitHub Check: CodeQL
source/tests/consistent/descriptor/test_dpa3.py
[notice] 127-127: Unused local variable
Variable update_residual_init is not used.
[notice] 128-128: Unused local variable
Variable exclude_types is not used.
[notice] 129-129: Unused local variable
Variable update_angle is not used.
[notice] 130-130: Unused local variable
Variable a_compress_rate is not used.
[notice] 131-131: Unused local variable
Variable a_compress_e_rate is not used.
[notice] 132-132: Unused local variable
Variable a_compress_use_split is not used.
[notice] 133-133: Unused local variable
Variable optim_update is not used.
[notice] 134-134: Unused local variable
Variable fix_stat_std is not used.
[notice] 135-135: Unused local variable
Variable n_multi_edge_message is not used.
[notice] 136-136: Unused local variable
Variable precision is not used.
[notice] 143-143: Unused local variable
Variable update_residual_init is not used.
[notice] 144-144: Unused local variable
Variable exclude_types is not used.
[notice] 145-145: Unused local variable
Variable update_angle is not used.
[notice] 146-146: Unused local variable
Variable a_compress_rate is not used.
[notice] 147-147: Unused local variable
Variable a_compress_e_rate is not used.
[notice] 148-148: Unused local variable
Variable a_compress_use_split is not used.
[notice] 149-149: Unused local variable
Variable optim_update is not used.
[notice] 150-150: Unused local variable
Variable fix_stat_std is not used.
[notice] 151-151: Unused local variable
Variable n_multi_edge_message is not used.
[notice] 152-152: Unused local variable
Variable precision is not used.
[notice] 160-160: Unused local variable
Variable update_residual_init is not used.
[notice] 161-161: Unused local variable
Variable exclude_types is not used.
deepmd/pt/model/descriptor/repflow_layer.py
[notice] 307-307: Unused local variable
Variable e_dim is not used.
[notice] 534-534: Unused local variable
Variable nall is not used.
[notice] 788-788: Unused local variable
Variable nitem is not used.
deepmd/dpmodel/descriptor/dpa3.py
[notice] 587-587: Unused local variable
Variable env_mat is not used.
deepmd/dpmodel/descriptor/repflows.py
[notice] 863-863: Unused local variable
Variable nall is not used.
[notice] 1125-1125: Unused local variable
Variable nitem is not used.
deepmd/pt/model/descriptor/dpa3.py
[notice] 423-423: Unused local variable
Variable env_mat is not used.
[notice] 479-479: Unused local variable
Variable nall is not used.
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: build_docker (_cu11, 11)
- GitHub Check: build_docker (12)
- GitHub Check: Test Python (6, 3.12)
- GitHub Check: Test Python (6, 3.9)
- GitHub Check: Test Python (5, 3.12)
- GitHub Check: Test Python (5, 3.9)
- GitHub Check: Test Python (4, 3.12)
- GitHub Check: Test Python (4, 3.9)
- GitHub Check: Test Python (3, 3.12)
- GitHub Check: Test Python (3, 3.9)
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Test Python (2, 3.9)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Test Python (1, 3.12)
- GitHub Check: Test C++ (false)
- GitHub Check: Test Python (1, 3.9)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Test C++ (true)
🔇 Additional comments (15)
deepmd/jax/descriptor/repflows.py (1)
43-62: Overall class structure appears well-implemented.The custom overrides for
__setattr__inRepFlowLayerconsistently handle deserialization of diverse layer attributes. This approach should help maintain clarity and consistency across different layer types.deepmd/pt/model/descriptor/dpa3.py (1)
66-573: Solid overall implementation and integration.Aside from the minor static analysis issues, the class comprehensively covers descriptor initialization, message passing, environment handling, and serialization. The
share_paramsmechanism and type-map updates appear robust and well-tested. Good job ensuring the descriptor is self-contained and accommodates future enhancements.🧰 Tools
🪛 Ruff (0.8.2)
75-75: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
347-347: Loop control variable
iinot used within loop bodyRename unused
iito_ii(B007)
423-423: Local variable
env_matis assigned to but never usedRemove assignment to unused variable
env_mat(F841)
479-479: Local variable
nallis assigned to but never usedRemove assignment to unused variable
nall(F841)
🪛 GitHub Check: CodeQL
[notice] 423-423: Unused local variable
Variable env_mat is not used.
[notice] 479-479: Unused local variable
Variable nall is not used.deepmd/pt/model/descriptor/repflows.py (1)
1-604: Well-structured design with comprehensive method coverage.The new
DescrptBlockRepflowsclass features a clear initialization flow, separation of logic via helper methods, and integration with message passing layers. The forward method is robust, handling environment matrix processing, neighbor lists, and angle calculations. Overall, the code is neatly arranged and should be maintainable moving forward.🧰 Tools
🪛 Ruff (0.8.2)
99-99: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
359-359: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
460-460: Loop control variable
idxnot used within loop bodyRename unused
idxto_idx(B007)
570-574: Use ternary operator
sampled = merged() if callable(merged) else mergedinstead ofif-else-block(SIM108)
deepmd/dpmodel/descriptor/__init__.py (1)
8-10: LGTM!The import statement and
__all__list update forDescrptDPA3follow the established pattern and maintain consistency with other descriptors.Also applies to: 36-36
deepmd/jax/descriptor/__init__.py (1)
8-10: LGTM!The import statement and
__all__list update forDescrptDPA3follow the established pattern and maintain consistency with other descriptors.Also applies to: 33-33
deepmd/pt/model/descriptor/__init__.py (1)
16-18: LGTM! Clean integration of the DPA3 descriptor.The import statement and
__all__list addition follow the established pattern and maintain alphabetical order.Also applies to: 55-55
source/tests/universal/pt/descriptor/test_descriptor.py (1)
7-7: LGTM! Comprehensive test coverage for DPA3 descriptor.The changes properly integrate DPA3 into the test framework, ensuring it's tested alongside other descriptors.
Also applies to: 24-24, 45-45
source/tests/array_api_strict/descriptor/repflows.py (2)
22-36: LGTM! Clean implementation of DescrptBlockRepflows.The class properly handles array conversion and deserialization of attributes, with clear handling of special cases.
39-56: LGTM! Comprehensive implementation of RepFlowLayer.The class handles all neural network layer attributes correctly, with proper deserialization and array conversion.
deepmd/pt/utils/utils.py (1)
42-43: LGTM! Clean addition of SiLU activation function.The implementation correctly uses PyTorch's
F.siluand follows the established pattern for activation functions.deepmd/dpmodel/utils/network.py (1)
321-328:⚠️ Potential issueFix incorrect SiLU implementation.
The current implementation of SiLU (Sigmoid Linear Unit) is incorrect. SiLU, also known as Swish, should be implemented as x * sigmoid(x).
elif activation_function == "silu": def fn(x): xp = array_api_compat.array_namespace(x) - # generated by GitHub Copilot - return x / (1 + xp.exp(-x)) + return x * (1 / (1 + xp.exp(-x))) # x * sigmoid(x) return fnLikely an incorrect or invalid review comment.
source/tests/universal/pt/model/test_model.py (3)
13-13: LGTM! Import statements for DPA3 descriptor added correctly.The new imports for
DescrptDPA3,DescriptorParamDPA3, andDescriptorParamDPA3Listare properly integrated with the existing imports.Also applies to: 59-60
99-99: LGTM! DPA3 descriptor param added to default parameters.The
DescriptorParamDPA3is correctly added to the default descriptor parameters list.
124-124: LGTM! DPA3 descriptor properly integrated into test parameterization.The
DescrptDPA3descriptor and its parameters are correctly added to the parameterized tests across all model test classes, maintaining consistency with the existing test structure.Also applies to: 139-139, 228-228, 243-243, 327-327, 338-338, 422-422, 433-433, 736-736, 747-747, 829-829, 839-839
deepmd/utils/argcheck.py (1)
1357-1422: LGTM! Well-structured descriptor registration with clear documentation.The DPA3 descriptor registration follows the established pattern and includes comprehensive argument definitions with appropriate types, defaults and documentation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
deepmd/dpmodel/descriptor/repflows.py (2)
338-349: Implementation placeholder for statistics.Both
compute_input_statsandget_statsraiseNotImplementedError. If these are planned features, consider implementing them or remove them if they're no longer needed.Would you like me to propose an interim implementation or open a new issue to track this as a TODO?
436-436: Rename unused loop variable for clarity.The loop variable
idxis not used, so renaming it to_or removing it may improve clarity.- for idx, ll in enumerate(self.layers): + for _, ll in enumerate(self.layers): ...🧰 Tools
🪛 Ruff (0.8.2)
436-436: Loop control variable
idxnot used within loop bodyRename unused
idxto_idx(B007)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deepmd/dpmodel/descriptor/repflows.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
deepmd/dpmodel/descriptor/repflows.py
77-77: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
352-352: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
436-436: Loop control variable idx not used within loop body
Rename unused idx to _idx
(B007)
927-927: Local variable nall is assigned to but never used
Remove assignment to unused variable nall
(F841)
1189-1189: Local variable nitem is assigned to but never used
Remove assignment to unused variable nitem
(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.12)
- GitHub Check: Test Python (5, 3.9)
- GitHub Check: Test Python (4, 3.12)
- GitHub Check: Test Python (4, 3.9)
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Test Python (3, 3.12)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Test Python (3, 3.9)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Test Python (2, 3.9)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Build C library (2.14, >=2.5.0rc0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Test Python (1, 3.12)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Test Python (1, 3.9)
🔇 Additional comments (5)
deepmd/dpmodel/descriptor/repflows.py (5)
77-77: Avoid mutable default argument in constructor.This constructor parameter
exclude_types: list[tuple[int, int]] = []uses a mutable default list, which can lead to unexpected behavior across invocations.Below is a suggested fix:
- exclude_types: list[tuple[int, int]] = [], + exclude_types: Optional[list[tuple[int, int]]] = None, ) -> None: + if exclude_types is None: + exclude_types = [] self.exclude_types = exclude_types🧰 Tools
🪛 Ruff (0.8.2)
77-77: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
352-352: Avoid mutable default argument in method definition.Similar to the constructor, the parameter
exclude_types: list[tuple[int, int]] = []can cause shared state issues.Below is a suggested fix:
- def reinit_exclude( - self, - exclude_types: list[tuple[int, int]] = [], - ) -> None: + def reinit_exclude( + self, + exclude_types: Optional[list[tuple[int, int]]] = None, + ) -> None: + if exclude_types is None: + exclude_types = [] self.exclude_types = exclude_types🧰 Tools
🪛 Ruff (0.8.2)
352-352: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
927-927: Remove unused local variable.Variable
nallis assigned but never used after assignment.- nall = node_ebd_ext.shape[1] node_ebd = node_ebd_ext[:, :nloc, :]🧰 Tools
🪛 Ruff (0.8.2)
927-927: Local variable
nallis assigned to but never usedRemove assignment to unused variable
nall(F841)
1189-1189: Remove unused local variable.Variable
nitemis assigned but never used.- nitem = len(update_list) uu = update_list[0]🧰 Tools
🪛 Ruff (0.8.2)
1189-1189: Local variable
nitemis assigned to but never usedRemove assignment to unused variable
nitem(F841)
1-1: Overall well-structured implementation.Aside from the minor issues noted above, the file is well-organized and follows a clean approach to implementing a descriptor block with multiple layers. Great job!
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
source/tests/universal/dpmodel/descriptor/test_descriptor.py (1)
474-474:⚠️ Potential issueReplace mutable default argument.
Using mutable default arguments in Python can lead to unexpected behavior. Replace the empty list default argument with
Noneand initialize it within the function.-def DescriptorParamDPA3(..., exclude_types=[], ...): +def DescriptorParamDPA3(..., exclude_types=None, ...): input_dict = { "repflow": RepFlowArgs( **{ ... } ), - "exclude_types": exclude_types, + "exclude_types": [] if exclude_types is None else exclude_types, ... } return input_dict🧰 Tools
🪛 Ruff (0.8.2)
474-474: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
deepmd/dpmodel/descriptor/repflows.py (2)
352-352: 🛠️ Refactor suggestionAvoid mutable default argument in
reinit_exclude.Similarly, using an empty list for
exclude_typesmay lead to shared references across calls. Replace withNoneand initialize locally to prevent subtle bugs.- def reinit_exclude( - self, - exclude_types: list[tuple[int, int]] = [], - ) -> None: + def reinit_exclude( + self, + exclude_types: Optional[list[tuple[int, int]]] = None, + ) -> None: + if exclude_types is None: + exclude_types = [] self.exclude_types = exclude_types🧰 Tools
🪛 Ruff (0.8.2)
352-352: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
77-77: 🛠️ Refactor suggestionAvoid mutable default argument for
exclude_types.Using
list[tuple[int,int]] = []as a default can inadvertently share state between instances. Consider adopting aNonedefault and initializing the list in the method body.- exclude_types: list[tuple[int, int]] = [], + exclude_types: Optional[list[tuple[int, int]]] = None, … ) -> None: + if exclude_types is None: + exclude_types = [] self.exclude_types = exclude_types🧰 Tools
🪛 Ruff (0.8.2)
77-77: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
🧹 Nitpick comments (5)
deepmd/dpmodel/descriptor/repflows.py (5)
53-83: Consider splitting the constructor into smaller methods.This constructor accepts a large number of parameters, making it somewhat cumbersome to maintain and test. Refactoring smaller logical parts (e.g., parsing edge/angle configurations, setting up embeddings, etc.) into helper methods would improve readability and maintainability.
🧰 Tools
🪛 Ruff (0.8.2)
77-77: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
438-438: Rename the unused loop index.Since
idxis never referenced inside the loop, renaming it to_underscores its unused nature and improves clarity.-for idx, ll in enumerate(self.layers): +for _, ll in enumerate(self.layers): node_ebd_ext = xp_take_along_axis(node_ebd, mapping, axis=1) ...🧰 Tools
🪛 Ruff (0.8.2)
438-438: Loop control variable
idxnot used within loop bodyRename unused
idxto_idx(B007)
870-1169: Consider refactoring thecallmethod to enhance readability.This method handles a substantial portion of message-passing logic, neighbor/angle computations, and embeddings. Extracting smaller sub-methods (e.g. edge update, angle update, node update) can improve clarity and testability.
🧰 Tools
🪛 Ruff (0.8.2)
929-929: Local variable
nallis assigned to but never usedRemove assignment to unused variable
nall(F841)
929-929: Remove the unused local variablenall.The variable
nallis never used after its assignment. Removing it avoids confusion and minor memory overhead.- nall = node_ebd_ext.shape[1] node_ebd = node_ebd_ext[:, :nloc, :]🧰 Tools
🪛 Ruff (0.8.2)
929-929: Local variable
nallis assigned to but never usedRemove assignment to unused variable
nall(F841)
1191-1191: Remove the unused local variablenitem.
nitemis assigned but never referenced. Removing it will help eliminate potential confusion about code intentions.- nitem = len(update_list) uu = update_list[0]🧰 Tools
🪛 Ruff (0.8.2)
1191-1191: Local variable
nitemis assigned to but never usedRemove assignment to unused variable
nitem(F841)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deepmd/dpmodel/descriptor/repflows.py(1 hunks)source/tests/universal/dpmodel/descriptor/test_descriptor.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
source/tests/universal/dpmodel/descriptor/test_descriptor.py
474-474: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
deepmd/dpmodel/descriptor/repflows.py
77-77: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
352-352: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
438-438: Loop control variable idx not used within loop body
Rename unused idx to _idx
(B007)
929-929: Local variable nall is assigned to but never used
Remove assignment to unused variable nall
(F841)
1191-1191: Local variable nitem is assigned to but never used
Remove assignment to unused variable nitem
(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.12)
- GitHub Check: Test Python (5, 3.9)
- GitHub Check: Test Python (4, 3.12)
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Test Python (4, 3.9)
- GitHub Check: Test Python (3, 3.12)
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Test Python (3, 3.9)
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Test Python (2, 3.9)
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Test Python (1, 3.12)
- GitHub Check: Build C++ (cuda, cuda)
- GitHub Check: Build C++ (cpu, cpu)
- GitHub Check: Test Python (1, 3.9)
🔇 Additional comments (4)
source/tests/universal/dpmodel/descriptor/test_descriptor.py (3)
10-10: LGTM!The imports are correctly organized and placed in the appropriate sections.
Also applies to: 21-23
498-500: Verify angle cutoff ratios.The angle cutoff values are set to half of the edge cutoffs (
rcut/2andrcut_smth/2), and the angle selection is set to a quarter of the edge selection (sel//4). Please verify these ratios are intentional and optimal for the DPA3 descriptor's performance.
529-546: LGTM!The parameterized test configurations are comprehensive and well-structured, covering various combinations of parameters. The descriptor is correctly integrated into the test class.
Also applies to: 606-606
deepmd/dpmodel/descriptor/repflows.py (1)
210-216: Great approach for defining embedding layers.Initializing
edge_embdandangle_embdwithNativeLayerprovides a clean and flexible way to handle embedding transformations, ensuring consistency in precision, seeding, and parameter initialization.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
deepmd/dpmodel/descriptor/repflows.py (5)
338-348: Add implementation or document why these methods raise NotImplementedError.Both
compute_input_statsandget_statsmethods raiseNotImplementedErrorwithout explanation. If these are intended to be abstract methods, consider:
- Adding a docstring explaining why they're not implemented
- Using
@abstractmethoddecorator if they must be implemented by subclassesWould you like me to help implement these methods or add appropriate documentation?
929-929: Remove unused variable.The variable
nallis assigned but never used.Apply this diff:
- nall = node_ebd_ext.shape[1] node_ebd = node_ebd_ext[:, :nloc, :]🧰 Tools
🪛 Ruff (0.8.2)
929-929: Local variable
nallis assigned to but never usedRemove assignment to unused variable
nall(F841)
1191-1191: Remove unused variable.The variable
nitemis assigned but never used.Apply this diff:
- nitem = len(update_list) uu = update_list[0]🧰 Tools
🪛 Ruff (0.8.2)
1191-1191: Local variable
nitemis assigned to but never usedRemove assignment to unused variable
nitem(F841)
778-828: Document performance benefits of optimized methods.The optimized methods
optim_angle_updateandoptim_edge_updatewould benefit from docstrings explaining:
- How they optimize the computation
- Expected performance improvements
- Any trade-offs compared to non-optimized versions
Consider adding docstrings like:
def optim_angle_update(self, ...) -> np.ndarray: """Optimized version of angle update computation. This method improves performance by [explain optimization strategy]. Benchmarks show [X]% improvement over the non-optimized version. Note: Only used when optim_update=True. """Also applies to: 829-869
382-382: Improve assertion error messages.The assertions would be more helpful in debugging if they included descriptive error messages explaining the expected values.
Apply these diffs:
- assert list(atype_embd.shape) == [nframes, nloc, self.n_dim] + assert list(atype_embd.shape) == [nframes, nloc, self.n_dim], \ + f"Expected shape [{nframes}, {nloc}, {self.n_dim}], got {list(atype_embd.shape)}"- assert (a_dim * a_compress_e_rate) % (2 * a_compress_rate) == 0, ( - f"For a_compress_rate of {a_compress_rate}, a_dim*a_compress_e_rate must be divisible by {2 * a_compress_rate}. " - f"Currently, a_dim={a_dim} and a_compress_e_rate={a_compress_e_rate} is not valid." + assert (a_dim * a_compress_e_rate) % (2 * a_compress_rate) == 0, \ + f"Invalid compression parameters: a_dim ({a_dim}) * a_compress_e_rate ({a_compress_e_rate}) " \ + f"must be divisible by 2 * a_compress_rate ({2 * a_compress_rate})"Also applies to: 586-589
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deepmd/dpmodel/descriptor/repflows.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
deepmd/dpmodel/descriptor/repflows.py
77-77: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
352-352: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
438-438: Loop control variable idx not used within loop body
Rename unused idx to _idx
(B007)
929-929: Local variable nall is assigned to but never used
Remove assignment to unused variable nall
(F841)
1191-1191: Local variable nitem is assigned to but never used
Remove assignment to unused variable nitem
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: Test Python (3, 3.12)
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Test Python (3, 3.9)
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Test Python (2, 3.9)
- GitHub Check: Build wheels for cp311-macosx_x86_64
- 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 Python (1, 3.12)
- GitHub Check: Test C++ (false)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Analyze (python)
- GitHub Check: Test C++ (true)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Test Python (1, 3.9)
- GitHub Check: Analyze (c-cpp)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
deepmd/dpmodel/descriptor/repflows.py (3)
438-438: Rename the unused loop variable for clarity.The variable
idxis never referenced inside the loop. Consider renaming it to_or_idxto make it clear it’s unused.-for idx, ll in enumerate(self.layers): +for _idx, ll in enumerate(self.layers): node_ebd_ext = xp_take_along_axis(node_ebd, mapping, axis=1) node_ebd, edge_ebd, angle_ebd = ll.call(...)🧰 Tools
🪛 Ruff (0.8.2)
438-438: Loop control variable
idxnot used within loop bodyRename unused
idxto_idx(B007)
357-467: Consider splitting thecallmethod to improve maintainability.This method has numerous steps for handling neighbor lists, building masks, computing angles, and updating embeddings, making it fairly large. Splitting it into smaller functions (e.g., one function for edge-related logic, another for angle-related logic) would enhance clarity and testability.
🧰 Tools
🪛 Ruff (0.8.2)
438-438: Loop control variable
idxnot used within loop bodyRename unused
idxto_idx(B007)
877-920: Refactor thecallmethod for clarity.Similar to the descriptor’s
call, this method handles a complex workflow: extracting neighbor embeddings, performing angle computations, partial updates, etc. Decomposing it into smaller, reusable sub-routines (e.g., self-update, edge-update, angle-update) can make maintenance easier and reduce cognitive load.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deepmd/dpmodel/descriptor/repflows.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
deepmd/dpmodel/descriptor/repflows.py
77-77: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
352-352: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
438-438: Loop control variable idx not used within loop body
Rename unused idx to _idx
(B007)
935-935: Local variable nall is assigned to but never used
Remove assignment to unused variable nall
(F841)
1197-1197: Local variable nitem is assigned to but never used
Remove assignment to unused variable nitem
(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.12)
- GitHub Check: Test Python (5, 3.9)
- GitHub Check: Test Python (4, 3.12)
- GitHub Check: Test Python (4, 3.9)
- GitHub Check: Test Python (3, 3.12)
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Test Python (3, 3.9)
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Test C++ (false)
- GitHub Check: Test Python (2, 3.9)
- GitHub Check: Build C++ (cuda, cuda)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build C++ (cpu, cpu)
- GitHub Check: Test Python (1, 3.12)
- GitHub Check: Test Python (1, 3.9)
🔇 Additional comments (4)
deepmd/dpmodel/descriptor/repflows.py (4)
77-77: Replace the mutable default argument with None.Using a list as a default argument could expose you to subtle bugs due to Python’s handling of mutable default values. Consider initializing it to
Noneand setting the list within the constructor body.Apply this diff to address the warning:
- exclude_types: list[tuple[int, int]] = [], + exclude_types: Optional[list[tuple[int, int]]] = None, ... ) -> None: + if exclude_types is None: + exclude_types = [] self.exclude_types = exclude_types🧰 Tools
🪛 Ruff (0.8.2)
77-77: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
352-352: Replace the mutable default argument with None.This method also uses a list as a default argument, which can cause unintended side effects. Initialize it to
Noneand handle the default inside the method body.Apply this diff:
- def reinit_exclude( - self, - exclude_types: list[tuple[int, int]] = [], - ) -> None: + def reinit_exclude( + self, + exclude_types: Optional[list[tuple[int, int]]] = None, + ) -> None: + if exclude_types is None: + exclude_types = [] self.exclude_types = exclude_types🧰 Tools
🪛 Ruff (0.8.2)
352-352: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
935-935: Remove unused local variable.The variable
nallis assigned but never used. Removing it avoids confusion and aligns with best practices.- nall = node_ebd_ext.shape[1] node_ebd = node_ebd_ext[:, :nloc, :]🧰 Tools
🪛 Ruff (0.8.2)
935-935: Local variable
nallis assigned to but never usedRemove assignment to unused variable
nall(F841)
1197-1197: Remove unused local variable.The variable
nitemis assigned but never used. Deleting it simplifies the method.- nitem = len(update_list) uu = update_list[0]🧰 Tools
🪛 Ruff (0.8.2)
1197-1197: Local variable
nitemis assigned to but never usedRemove assignment to unused variable
nitem(F841)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #4609 +/- ##
==========================================
+ Coverage 84.58% 84.78% +0.19%
==========================================
Files 680 688 +8
Lines 64547 66088 +1541
Branches 3539 3541 +2
==========================================
+ Hits 54600 56032 +1432
- Misses 8806 8915 +109
Partials 1141 1141 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deepmd/pt/model/descriptor/descriptor.py (1)
141-144: Good enhancement of parameter sharing logic.The addition of the compound condition to check
set_stddev_constantandset_davg_zeroattributes provides more fine-grained control over when parameters are recalculated during multitask training. This change properly supports scenarios where users might want to maintain constant standard deviation or zero mean values even when not resuming from a checkpoint.However, consider adding documentation comments that explain the purpose and intended usage of these attributes to improve code maintainability. This would help future developers understand when these attributes should be set and how they affect parameter recalculation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
deepmd/pt/model/descriptor/repflows.py (1)
358-364: 🛠️ Refactor suggestionAvoid using a mutable default argument for
exclude_types.This can cause implicit state sharing across function calls.
- def reinit_exclude(self, exclude_types: list[tuple[int, int]] = []): + def reinit_exclude(self, exclude_types: Optional[list[tuple[int, int]]] = None): + if exclude_types is None: + exclude_types = []🧰 Tools
🪛 Ruff (0.8.2)
360-360: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
deepmd/pt/model/descriptor/dpa3.py (1)
105-105: 🛠️ Refactor suggestionAvoid using a mutable default argument for
exclude_types.This prevents unintended side effects across calls.
- exclude_types: list[tuple[int, int]] = [], + exclude_types: Optional[list[tuple[int, int]]] = None, + ... + if exclude_types is None: + exclude_types = []🧰 Tools
🪛 Ruff (0.8.2)
105-105: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
🧹 Nitpick comments (6)
deepmd/pt/model/descriptor/repflows.py (2)
461-461: Rename the unused loop variableidxto_.This clarifies that
idxis not used and prevents lint warnings.- for idx, ll in enumerate(self.layers): + for _, ll in enumerate(self.layers):🧰 Tools
🪛 Ruff (0.8.2)
461-461: Loop control variable
idxnot used within loop bodyRename unused
idxto_idx(B007)
571-575: Use a ternary operator to simplify the conditional.This minor refactor improves readability.
- if callable(merged): - sampled = merged() - else: - sampled = merged + sampled = merged() if callable(merged) else merged🧰 Tools
🪛 Ruff (0.8.2)
571-575: Use ternary operator
sampled = merged() if callable(merged) else mergedinstead ofif-else-block(SIM108)
deepmd/pt/model/descriptor/dpa3.py (3)
339-339: Use_in place of the unused loop variableii.Renaming helps avoid confusion and lint warnings.
- for ii, descrpt in enumerate(descrpt_list): + for _, descrpt in enumerate(descrpt_list):🧰 Tools
🪛 Ruff (0.8.2)
339-339: Loop control variable
iinot used within loop bodyRename unused
iito_ii(B007)
415-416: Remove or utilize the unused local variableenv_mat.Leaving it unused can be confusing and clutters the code.
- env_mat = repflow_variable.pop("env_mat")🧰 Tools
🪛 Ruff (0.8.2)
415-415: Local variable
env_matis assigned to but never usedRemove assignment to unused variable
env_mat(F841)
471-471: Remove the unused local variablenall.Eliminating unused variables helps keep the code clean.
- nall = extended_coord.view(nframes, -1).shape[1] // 3🧰 Tools
🪛 Ruff (0.8.2)
471-471: Local variable
nallis assigned to but never usedRemove assignment to unused variable
nall(F841)
deepmd/dpmodel/descriptor/repflows.py (1)
439-439: Rename unused loop variable
The loop variableidxis not used. Renaming it to_or removing it can clarify that it’s not needed.-for idx, ll in enumerate(self.layers): +for _, ll in enumerate(self.layers): ...🧰 Tools
🪛 Ruff (0.8.2)
439-439: Loop control variable
idxnot used within loop bodyRename unused
idxto_idx(B007)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
deepmd/dpmodel/descriptor/dpa3.py(1 hunks)deepmd/dpmodel/descriptor/repflows.py(1 hunks)deepmd/jax/descriptor/dpa3.py(1 hunks)deepmd/jax/descriptor/repflows.py(1 hunks)deepmd/pt/model/descriptor/dpa3.py(1 hunks)deepmd/pt/model/descriptor/repflows.py(1 hunks)source/tests/array_api_strict/descriptor/dpa3.py(1 hunks)source/tests/array_api_strict/descriptor/repflows.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- source/tests/array_api_strict/descriptor/dpa3.py
- deepmd/jax/descriptor/repflows.py
- deepmd/jax/descriptor/dpa3.py
🧰 Additional context used
🪛 Ruff (0.8.2)
deepmd/pt/model/descriptor/repflows.py
178-178: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
360-360: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
461-461: Loop control variable idx not used within loop body
Rename unused idx to _idx
(B007)
571-575: Use ternary operator sampled = merged() if callable(merged) else merged instead of if-else-block
(SIM108)
deepmd/dpmodel/descriptor/dpa3.py
253-253: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
574-574: Local variable env_mat is assigned to but never used
Remove assignment to unused variable env_mat
(F841)
deepmd/dpmodel/descriptor/repflows.py
156-156: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
353-353: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
439-439: Loop control variable idx not used within loop body
Rename unused idx to _idx
(B007)
936-936: Local variable nall is assigned to but never used
Remove assignment to unused variable nall
(F841)
1198-1198: Local variable nitem is assigned to but never used
Remove assignment to unused variable nitem
(F841)
deepmd/pt/model/descriptor/dpa3.py
105-105: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
339-339: Loop control variable ii not used within loop body
Rename unused ii to _ii
(B007)
415-415: Local variable env_mat is assigned to but never used
Remove assignment to unused variable env_mat
(F841)
471-471: 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 (3, 3.9)
- GitHub Check: Test Python (5, 3.12)
- GitHub Check: Test Python (1, 3.12)
- GitHub Check: Test Python (5, 3.9)
- GitHub Check: Test Python (4, 3.12)
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Test Python (3, 3.12)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Build C++ (cuda, cuda)
- 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 cp310-manylinux_aarch64
- GitHub Check: Build C++ (cpu, cpu)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
🔇 Additional comments (9)
source/tests/array_api_strict/descriptor/repflows.py (2)
22-38: Implementation ofDescrptBlockRepflowsappears correct and straightforward.No major issues identified. The custom
__setattr__logic is well-organized and properly calls the parent class afterwards.
41-61: Implementation ofRepFlowLayeris consistent and logically sound.No major issues identified. The on-demand deserialization pattern helps ensure proper handling of layer attributes.
deepmd/pt/model/descriptor/repflows.py (1)
1-357: Overall implementation is robust and well-structured.The class successfully handles environment matrix computations, message passing, and statistics.
🧰 Tools
🪛 Ruff (0.8.2)
178-178: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
deepmd/pt/model/descriptor/dpa3.py (1)
1-104: File structure and class organization appear correct.No major issues are found in the initial lines.
deepmd/dpmodel/descriptor/dpa3.py (2)
253-253: UseNoneinstead of an empty list for default argument
This line uses a mutable data structure ([]) as a default. That can lead to subtle bugs if the list is modified.- exclude_types: list[tuple[int, int]] = [], + exclude_types: Optional[list[tuple[int, int]]] = None, … ) -> None: + if exclude_types is None: + exclude_types = [] self.exclude_types = exclude_types🧰 Tools
🪛 Ruff (0.8.2)
253-253: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
574-574: Remove unused local variable
Variableenv_matis assigned but never used, which can introduce confusion.- env_mat = repflow_variable.pop("env_mat")🧰 Tools
🪛 Ruff (0.8.2)
574-574: Local variable
env_matis assigned to but never usedRemove assignment to unused variable
env_mat(F841)
deepmd/dpmodel/descriptor/repflows.py (3)
156-156: Avoid mutable default argument
Using a list literal as a default can cause unexpected shared state between calls. Replace it withNoneand initialize inside the method.- exclude_types: list[tuple[int, int]] = [], + exclude_types: Optional[list[tuple[int, int]]] = None, … ) -> None: + if exclude_types is None: + exclude_types = [] self.exclude_types = exclude_types🧰 Tools
🪛 Ruff (0.8.2)
156-156: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
936-936: Remove unused local variable
nallis assigned but never used, which can lead to confusion.- nall = node_ebd_ext.shape[1]🧰 Tools
🪛 Ruff (0.8.2)
936-936: Local variable
nallis assigned to but never usedRemove assignment to unused variable
nall(F841)
1198-1198: Remove unused local variable
nitemis assigned but never used, unnecessarily cluttering the code.- nitem = len(update_list)🧰 Tools
🪛 Ruff (0.8.2)
1198-1198: Local variable
nitemis assigned to but never usedRemove assignment to unused variable
nitem(F841)
|
@iProzd Nice work! I wonder if |
Yes, you can use it in this PR. It's a common method in all descriptors. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
deepmd/pt/model/task/fitting.py (1)
95-97: Optimization for cases with no parameters.The added condition improves efficiency by skipping unnecessary statistical computations when no frame or atomic parameters are present. This is particularly relevant for the new DPA3 descriptor which may operate in scenarios without these parameters.
While the current implementation works well, you might want to enhance the comment to be more specific about why statistics computation is being skipped:
- # skip data statistics + # Skip data statistics computation since there are no frame or atomic parameters to processsource/tests/pt/test_multitask.py (2)
404-405: Consider using a unique directory name for test artifacts.The test is using "DPA3" as the directory name for storing test artifacts. Since this is a new model, using a more specific name like "DPA3_multitask" might help avoid potential conflicts with other tests that might use the same directory name.
- self.stat_files = "DPA3" + self.stat_files = "DPA3_multitask"
425-426:❓ Verification inconclusive
Consider increasing test steps for better coverage.
The test is configured to run only 1 step. While this is consistent with other tests and suitable for basic functionality testing, consider whether a slightly higher number of steps might provide better confidence in the model's training stability with the new descriptor.
🏁 Script executed:
#!/bin/bash # Check if other test files use different number of steps for DPA-related tests rg "numb_steps.*[2-9]" --glob "*dpa*.py"Length of output: 42
Revisit Test Steps for Expanded Coverage
Currently, the test in
source/tests/pt/test_multitask.pysets:self.config["training"]["numb_steps"] = 1 self.config["training"]["save_freq"] = 1This configuration is consistent with other tests—as our initial search (including DPA-related files) didn’t reveal any alternative (higher step) settings. That said, while a single-step run is sufficient for basic functionality, increasing the number of steps could yield better insights into the training stability of the new descriptor.
Please manually verify whether a higher step count might expose edge cases or improve confidence in training stability without unduly lengthening test runtimes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
deepmd/pt/model/task/fitting.py(1 hunks)source/tests/pt/model/test_permutation.py(1 hunks)source/tests/pt/test_multitask.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Test Python (3, 3.12)
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Test Python (3, 3.9)
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Test Python (2, 3.9)
- GitHub Check: Analyze (python)
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Test Python (1, 3.12)
- 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: Analyze (c-cpp)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Test Python (1, 3.9)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Test C++ (false)
- GitHub Check: Test C++ (true)
🔇 Additional comments (5)
source/tests/pt/model/test_permutation.py (3)
170-206: Well-structured DPA3 model configuration added.The new
model_dpa3configuration follows the established pattern of other descriptor models in this test file. It includes appropriate parameters for the "dpa3" descriptor type with detailedrepflowcomponent settings.
196-196:❓ Verification inconclusive
Consider standardizing precision settings across models.
This model explicitly specifies "float32" precision while other models in the file use the default precision. Consider whether this difference is intentional and whether precision should be standardized across all models for consistent testing.
🏁 Script executed:
#!/bin/bash # Check for other precision settings in the codebase rg "precision.*float32" --glob "*.py"Length of output: 3582
Standardize Precision Across Models for Consistency
Based on our repository-wide search, nearly all tests explicitly set the precision to
"float32"(including in other tests within the same directory), so the explicit setting insource/tests/pt/model/test_permutation.pyappears consistent with common usage. It is unlikely this is an accidental divergence. However, if the intention was to rely on a default behavior (to reduce redundancy or centralize configuration), it might be worth considering removing the explicit"precision": "float32"setting and using the default defined elsewhere. If that change is not desired, no further action is required.
- Action Items:
- Confirm if the explicit setting is intentional.
- If a unified default is preferred, update or remove similar hard-coded precision settings across tests for better consistency.
194-195:✅ Verification successful
Verify SiLU activation function implementation.
The DPA3 model uses "silu" activation function, which is different from most other models in this file that use "tanh". Make sure the SiLU activation function is properly implemented and tested in the relevant code.
🏁 Script executed:
#!/bin/bash # Check if SiLU activation function is implemented in the codebase rg -A 2 "silu|SiLU" --glob "*.py" | grep -v "test_"Length of output: 3067
SiLU activation function verified
The search output confirms that the SiLU activation function is implemented correctly (e.g., indeepmd/tf/common.pyand referenced in several other modules likedeepmd/pt/utils/utils.pyand the DPA3 descriptor). The tests insource/tests/pt/model/test_permutation.pythat reference"activation_function": "silu"now clearly reflect the intended implementation. Please ensure that the tests adequately validate its behavior, especially given its differences from the usual"tanh"activation elsewhere.source/tests/pt/test_multitask.py (2)
35-35: Appropriate import of model_dpa3.The import statement has been correctly updated to include the new
model_dpa3configuration.
397-434: Well-structured test class for DPA3 multitask capabilities.The
TestMultiTaskDPA3class follows the consistent pattern established by other multitask test classes. It properly configures the test with the DPA3 descriptor model and sets up appropriate test data paths and parameters.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deepmd/dpmodel/descriptor/repflows.py (1)
440-440: Rename the unused loop variable to an underscore.The variable
idxis not used in this loop body, so rename it to_idxto reflect that it is intentionally unused.- for idx, ll in enumerate(self.layers): + for _idx, ll in enumerate(self.layers): node_ebd_ext = xp_take_along_axis(node_ebd, mapping, axis=1) ...🧰 Tools
🪛 Ruff (0.8.2)
440-440: Loop control variable
idxnot used within loop bodyRename unused
idxto_idx(B007)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deepmd/dpmodel/descriptor/repflows.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
deepmd/dpmodel/descriptor/repflows.py
156-156: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
353-353: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
440-440: Loop control variable idx not used within loop body
Rename unused idx to _idx
(B007)
937-937: Local variable nall is assigned to but never used
Remove assignment to unused variable nall
(F841)
1199-1199: Local variable nitem is assigned to but never used
Remove assignment to unused variable nitem
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- 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: Test Python (6, 3.12)
- GitHub Check: Test Python (6, 3.9)
- GitHub Check: Test Python (5, 3.12)
- GitHub Check: Test Python (5, 3.9)
- GitHub Check: Test Python (4, 3.12)
- GitHub Check: Test Python (4, 3.9)
- GitHub Check: Test Python (3, 3.12)
- 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.12)
- GitHub Check: Test Python (1, 3.9)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (3)
deepmd/dpmodel/descriptor/repflows.py (3)
156-156: Replace mutable default arguments withNone.Using a list literal (
[]) as a default argument can lead to unexpected behavior, because the list is created only once at function definition time. This is a repeat of a previously flagged issue. Consider usingNoneand initializing the list inside the function body instead.
[duplicate_comment, refactor_suggestion_essential]- exclude_types: list[tuple[int, int]] = [], + exclude_types: Optional[list[tuple[int, int]]] = None, ) -> None: + if exclude_types is None: + exclude_types = [] self.exclude_types = exclude_typesAlso applies to: 353-353
🧰 Tools
🪛 Ruff (0.8.2)
156-156: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
937-937: Remove unused local variable.The variable
nallis never used after assignment. This is a repeat of a previously flagged issue; removing it helps avoid confusion.
[duplicate_comment, refactor_suggestion_good_to_have]- nall = node_ebd_ext.shape[1] node_ebd = node_ebd_ext[:, :nloc, :]🧰 Tools
🪛 Ruff (0.8.2)
937-937: Local variable
nallis assigned to but never usedRemove assignment to unused variable
nall(F841)
1199-1199: Remove unused local variable.The variable
nitemis assigned but never used in this method. This is a repeat of a previously flagged issue; removing it simplifies the code.
[duplicate_comment, refactor_suggestion_good_to_have]- nitem = len(update_list) uu = update_list[0]🧰 Tools
🪛 Ruff (0.8.2)
1199-1199: Local variable
nitemis assigned to but never usedRemove assignment to unused variable
nitem(F841)
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
source/tests/universal/dpmodel/descriptor/test_descriptor.py (1)
467-526:⚠️ Potential issueReplace mutable default argument with None.
Using mutable default arguments in Python can lead to unexpected behavior. Replace the empty list default argument with
Noneand initialize it within the function.-def DescriptorParamDPA3(..., exclude_types=[], ...): +def DescriptorParamDPA3(..., exclude_types=None, ...): input_dict = { "repflow": RepFlowArgs( **{ ... } ), - "exclude_types": exclude_types, + "exclude_types": [] if exclude_types is None else exclude_types, ... } return input_dictThe function follows the same pattern as other descriptor parameter functions, which is good for consistency. The RepFlowArgs is correctly implemented with parameters specific to the DPA3 descriptor.
🧰 Tools
🪛 Ruff (0.8.2)
474-474: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
🧹 Nitpick comments (1)
source/tests/universal/dpmodel/descriptor/test_descriptor.py (1)
467-548: Add documentation comments for the DPA3 descriptor.Consider adding a brief docstring or comment explaining the purpose and characteristics of the DPA3 descriptor and how it differs from DPA1 and DPA2. This would be helpful for future maintainers of the code.
def DescriptorParamDPA3( ntypes, rcut, rcut_smth, sel, type_map, env_protection=0.0, exclude_types=[], update_style="res_residual", update_residual=0.1, update_residual_init="const", update_angle=True, n_multi_edge_message=1, a_compress_rate=0, a_compress_e_rate=1, a_compress_use_split=False, optim_update=True, fix_stat_std=0.3, precision="float64", ): + """ + Create parameters for the DPA3 descriptor. + + DPA3 is an advanced descriptor that builds upon DPA1 and DPA2 by implementing + a message-passing neural network with angle information and optimized updates. + """🧰 Tools
🪛 Ruff (0.8.2)
474-474: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
source/tests/universal/dpmodel/descriptor/test_descriptor.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
source/tests/universal/dpmodel/descriptor/test_descriptor.py
474-474: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Build C library (2.14, >=2.5.0rc0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Test C++ (false)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Test C++ (true)
🔇 Additional comments (5)
source/tests/universal/dpmodel/descriptor/test_descriptor.py (5)
10-10: Added import for DescrptDPA3.This is correctly added to the existing imports from the deepmd.dpmodel.descriptor module.
21-23: Added import for RepFlowArgs from the new dpa3 module.The import of RepFlowArgs from the dpa3 module looks good and is consistent with the pattern used for other descriptor types.
529-546: Good parameterization of DPA3 descriptor.The parameterization using OrderedDict provides a comprehensive set of testing configurations for the DPA3 descriptor, which is consistent with the approach used for other descriptors.
606-606: Successfully integrated DPA3 into test framework.The DPA3 descriptor has been properly integrated into the existing parameterized test class, ensuring it will be tested alongside the other descriptors.
517-517:✅ Verification successful
Note usage of SiLU activation function.
Unlike some other descriptors that use "tanh" as the activation function, this descriptor uses "silu" (Sigmoid Linear Unit, also known as Swish). This is likely intentional but worth noting as it's different from other descriptors in this file.
🏁 Script executed:
#!/bin/bash # Check where SiLU activation function is implemented in the codebase # Check for SiLU implementation rg -i "silu|swish" --type pyLength of output: 1481
Action: Confirmed usage of "silu" activation function is intentional and consistent.
After verifying the codebase with the search for "silu" or "swish", we can confirm that the SiLU activation function is widely used in various parts of the project (e.g., in files under deepmd/dpmodel, deepmd/pt, and deepmd/tf). Although the comment notes that some descriptors use "tanh", the implementation using "silu" aligns with the intended design in this descriptor and elsewhere.
- Verified File:
source/tests/universal/dpmodel/descriptor/test_descriptor.py(line 517) correctly sets"activation_function": "silu".- Consistency Check: Multiple files across the repository reference "silu" (or alias "swish"), confirming the intentional usage.
No changes are needed.
NOTE: examples, custom ops and docs will be updated in other PRs.
Summary by CodeRabbit
New Features
DescrptDPA3descriptor for advanced molecular simulations, enhancing flexibility and performance.model_dpa3for testing purposes.Tests
DescrptDPA3descriptor and repflow functionalities, ensuring consistency and reliability across different backends.DescrptDPA3to validate its functionality and performance across various configurations.DescrptDPA3to assess its behavior under different conditions.TestMultiTaskDPA3for multitask training scenarios involvingmodel_dpa3.