feat(pd): add se_atten_v2 #4558
Conversation
Signed-off-by: Jinzhe Zeng <[email protected]>
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
deepmd/pd/model/descriptor/se_atten_v2.py:43
- [nitpick] The parameter name 'sel' is ambiguous. It should be renamed to 'selection' for clarity.
sel: Union[list[int], int],
📝 WalkthroughWalkthroughThis pull request introduces a new descriptor class Changes
Sequence DiagramsequenceDiagram
participant Descriptor as DescrptSeAttenV2
participant Serializer as Serialization Methods
Descriptor->>Serializer: serialize()
Serializer-->>Descriptor: Returns state dictionary
Descriptor->>Serializer: deserialize(data)
Serializer-->>Descriptor: Reconstructs object instance
Possibly related PRs
Suggested labels
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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
deepmd/pd/model/descriptor/__init__.py(2 hunks)deepmd/pd/model/descriptor/se_atten_v2.py(1 hunks)source/tests/consistent/descriptor/test_se_atten_v2.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
deepmd/pd/model/descriptor/se_atten_v2.py
45-45: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
56-56: 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 (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: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Test Python (1, 3.9)
🔇 Additional comments (5)
deepmd/pd/model/descriptor/se_atten_v2.py (1)
195-275: Implementation ofserializeanddeserializemethodsThe
serializeanddeserializemethods are well-implemented, handling the conversion of the object's state to and from a dictionary correctly. This ensures that the descriptor can be saved and loaded effectively.deepmd/pd/model/descriptor/__init__.py (2)
25-27: Correctly added import forDescrptSeAttenV2The addition of
DescrptSeAttenV2to the imports ensures that the new descriptor is properly included in the module.
43-43: Updated__all__to includeDescrptSeAttenV2Including
DescrptSeAttenV2in the__all__list makes it publicly accessible when the module is imported, which is appropriate for this new descriptor.source/tests/consistent/descriptor/test_se_atten_v2.py (2)
48-53: Conditional import for Paddle backend is appropriateThe conditional import of
DescrptSeAttenV2for the Paddle backend (pd) correctly ensures that the class is only imported when Paddle is installed.
378-387: Addedeval_pdmethod for Paddle backend testingThe
eval_pdmethod is correctly implemented to evaluate the descriptor using the Paddle backend, ensuring consistency in testing across different backends.
Signed-off-by: Jinzhe Zeng <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
deepmd/pd/model/descriptor/se_atten_v2.py (1)
45-45:⚠️ Potential issueFix mutable default arguments.
Using mutable default values for
neuronandexclude_typesparameters can lead to unexpected behavior as these defaults are shared across all instances. UseNoneas the default and initialize the lists in the constructor.Apply this diff to fix both issues:
- neuron: list = [25, 50, 100], + neuron: Optional[list] = None, # ... other parameters ... - exclude_types: list[tuple[int, int]] = [], + exclude_types: Optional[list[tuple[int, int]]] = None,Then initialize them in the constructor:
def __init__(self, ...): if neuron is None: neuron = [25, 50, 100] if exclude_types is None: exclude_types = [] # ... rest of the initializationAlso applies to: 56-56
🧰 Tools
🪛 Ruff (0.8.2)
45-45: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
🧹 Nitpick comments (3)
deepmd/pd/model/descriptor/se_atten_v2.py (3)
75-155: Improve docstring formatting for better readability.The docstring has comprehensive parameter descriptions but could benefit from better formatting:
- The math equations could use consistent formatting
- Some parameter descriptions could be more concise
Consider reformatting math equations consistently, for example:
:math:r_c`` →:math:r_{\text{c}}`:math:r_s`` →:math:r_{\text{s}}`
195-242: Add validation for critical fields in serialization.While the serialization is comprehensive, consider adding validation for critical fields to ensure data integrity:
def serialize(self) -> dict: obj = self.se_atten + # Validate critical fields + if obj.rcut <= 0: + raise ValueError("rcut must be positive") + if obj.rcut_smth >= obj.rcut: + raise ValueError("rcut_smth must be less than rcut") + if obj.ntypes <= 0: + raise ValueError("ntypes must be positive") data = { "@class": "Descriptor", # ... rest of the serialization
260-261: Enhance tensor conversion with error handling.The tensor conversion function could be more robust by adding error handling and type checking:
def t_cvt(xx): + if not isinstance(xx, (numpy.ndarray, list)): + raise TypeError(f"Expected numpy array or list, got {type(xx)}") + try: return paddle.to_tensor(xx, dtype=obj.se_atten.prec, place=env.DEVICE) + except Exception as e: + raise ValueError(f"Failed to convert tensor: {e}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deepmd/pd/model/descriptor/se_atten_v2.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
deepmd/pd/model/descriptor/se_atten_v2.py
45-45: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
56-56: 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 (20)
- 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: Test Python (3, 3.9)
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Test Python (2, 3.9)
- GitHub Check: Analyze (python)
- GitHub Check: Test Python (1, 3.12)
- 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: Test C++ (false)
- GitHub Check: Build C++ (cpu, cpu)
- GitHub Check: Test Python (1, 3.9)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Test C++ (true)
🔇 Additional comments (1)
deepmd/pd/model/descriptor/se_atten_v2.py (1)
1-38: LGTM! Well-organized imports and proper class registration.The imports are logically grouped and the class is properly registered using the decorator pattern.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #4558 +/- ##
=======================================
Coverage 84.60% 84.61%
=======================================
Files 680 681 +1
Lines 64468 64513 +45
Branches 3538 3538
=======================================
+ Hits 54546 54588 +42
- Misses 8781 8783 +2
- Partials 1141 1142 +1 ☔ View full report in Codecov by Sentry. |
Has been used for benchmark
Summary by CodeRabbit
Release Notes
New Features
DescrptSeAttenV2, with advanced embedding and attention mechanisms.Tests
pdbackend.