feat(dp/pt): refactor se_e3 descriptor
#3813
Conversation
WalkthroughThe recent changes introduce a new class Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant DescrptSeT
participant DeepPot-SE Frameworks
User->>DescrptSeT: Initialize with parameters
DescrptSeT->>DeepPot-SE Frameworks: Compute descriptor
DeepPot-SE Frameworks-->>DescrptSeT: Return descriptor
DescrptSeT-->>User: Provide descriptor
Possibly related issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Outside diff range and nitpick comments (3)
source/tests/pt/model/test_se_e2_a.py (2)
Line range hint
59-105: Thetest_consistencymethod is well-implemented, covering various configurations for the descriptor. However, consider removing unused variables such asidt,prec, andemto clean up the code.- for idt, prec, em in itertools.product( + for _, _, _ in itertools.product(
Line range hint
107-134: Thetest_jitmethod correctly tests JIT compilation. However, the variablemodelis redundantly assigned twice. Consider simplifying this.- model = torch.jit.script(dd0) - model = torch.jit.script(dd1) + model = torch.jit.script(dd1)deepmd/tf/descriptor/se_t.py (1)
758-759: Clarify the requirement forndimto be 2 in the documentation.The requirement for
ndimto be 2 is enforced via an assertion in theserialize_networkmethod. It would be beneficial for maintainability and clarity to mention this requirement in the method's documentation.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #3813 +/- ##
========================================
Coverage 82.53% 82.53%
========================================
Files 513 515 +2
Lines 49040 49492 +452
Branches 2985 2985
========================================
+ Hits 40473 40849 +376
- Misses 7656 7732 +76
Partials 911 911 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Jinzhe Zeng <[email protected]> Signed-off-by: Duo <[email protected]>
|
A UT is needed for testing consistency between results using |
| data.pop("type", None) | ||
| variables = data.pop("@variables") | ||
| embeddings = data.pop("embeddings") | ||
| env_mat = data.pop("env_mat") |
Check notice
Code scanning / CodeQL
Unused local variable Note
| data.pop("type", None) | ||
| variables = data.pop("@variables") | ||
| embeddings = data.pop("embeddings") | ||
| env_mat = data.pop("env_mat") |
Check notice
Code scanning / CodeQL
Unused local variable Note
| @property | ||
| def skip_pt(self) -> bool: | ||
| ( | ||
| resnet_dt, |
Check notice
Code scanning / CodeQL
Unused local variable Note test
| def skip_pt(self) -> bool: | ||
| ( | ||
| resnet_dt, | ||
| excluded_types, |
Check notice
Code scanning / CodeQL
Unused local variable Note test
| ( | ||
| resnet_dt, | ||
| excluded_types, | ||
| precision, |
Check notice
Code scanning / CodeQL
Unused local variable Note test
| # TF se_e2_a type_one_side=False requires atype sorted | ||
| ( | ||
| resnet_dt, | ||
| excluded_types, |
Check notice
Code scanning / CodeQL
Unused local variable Note test
| ( | ||
| resnet_dt, | ||
| excluded_types, | ||
| precision, |
Check notice
Code scanning / CodeQL
Unused local variable Note test
| resnet_dt, | ||
| excluded_types, | ||
| precision, | ||
| env_protection, |
Check notice
Code scanning / CodeQL
Unused local variable Note test
| dd0.seat.mean = torch.tensor(davg, dtype=dtype, device=env.DEVICE) | ||
| dd0.seat.dstd = torch.tensor(dstd, dtype=dtype, device=env.DEVICE) | ||
| dd1 = DescrptSeT.deserialize(dd0.serialize()) | ||
| model = torch.jit.script(dd0) |
Check warning
Code scanning / CodeQL
Variable defined multiple times Warning test
| dd0.seat.dstd = torch.tensor(dstd, dtype=dtype, device=env.DEVICE) | ||
| dd1 = DescrptSeT.deserialize(dd0.serialize()) | ||
| model = torch.jit.script(dd0) | ||
| model = torch.jit.script(dd1) |
Check warning
Code scanning / CodeQL
Variable defined multiple times Warning test
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
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Note: 1. `exclude_types` is supported only for pt/dp. 2. Note that an exsiting bug in TF is fixed in deepmd/tf/env.py, when `resnet_dt` is `True` for `se_e3`. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced `DescrptSeT` class for DeepPot-SE descriptor with enhanced atomic configuration handling. - Added `DescrptBlockSeT` class for descriptor block functionality. - **Improvements** - Enhanced parameter management and serialization methods in descriptor classes. - Added `env_protection` parameter for better environmental control. - **Bug Fixes** - Improved method signatures for better consistency and error handling. - **Tests** - Added comprehensive test cases for `DescrptSeT` class across different deep learning frameworks. - Included consistency checks and JIT compilation tests. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Duo <[email protected]> Co-authored-by: Jinzhe Zeng <[email protected]> Co-authored-by: Han Wang <[email protected]>
Note:
exclude_typesis supported only for pt/dp.resnet_dtisTrueforse_e3.Summary by CodeRabbit
New Features
DescrptSeTclass for DeepPot-SE descriptor with enhanced atomic configuration handling.DescrptBlockSeTclass for descriptor block functionality.Improvements
env_protectionparameter for better environmental control.Bug Fixes
Tests
DescrptSeTclass across different deep learning frameworks.