Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: add se_r descriptor #3338

Merged
merged 23 commits into from
Feb 27, 2024
Merged

Feat: add se_r descriptor #3338

merged 23 commits into from
Feb 27, 2024

Conversation

anyangml
Copy link
Collaborator

@anyangml anyangml commented Feb 26, 2024

This PR is to support se_r descriptor in pytorch and numpy.

  • Refactor Pytorch env_mat: possibly combine r and a.
  • Add numpy implementation.
  • Add consistency test with tf.
  • Refactor device as parameter

@anyangml anyangml marked this pull request as draft February 26, 2024 04:01
deepmd/pt/model/descriptor/se_r.py Fixed Show fixed Hide fixed
deepmd/pt/model/descriptor/se_r.py Fixed Show fixed Hide fixed
deepmd/pt/model/descriptor/se_r.py Fixed Show fixed Hide fixed
source/tests/pt/model/test_descriptor_se_r.py Fixed Show fixed Hide fixed
source/tests/pt/model/test_descriptor_se_r.py Fixed Show fixed Hide fixed
deepmd/pt/model/descriptor/env_mat.py Fixed Show fixed Hide fixed
data = data.copy()
variables = data.pop("@variables")
embeddings = data.pop("embeddings")
env_mat = data.pop("env_mat")

Check notice

Code scanning / CodeQL

Unused local variable Note

Variable env_mat is not used.
source/tests/pt/model/test_descriptor_se_r.py Fixed Show fixed Hide fixed
source/tests/pt/model/test_descriptor_se_r.py Fixed Show fixed Hide fixed
source/tests/pt/model/test_descriptor_se_r.py Fixed Show fixed Hide fixed
Copy link

codecov bot commented Feb 26, 2024

Codecov Report

Attention: Patch coverage is 84.71338% with 48 lines in your changes are missing coverage. Please review.

Project coverage is 75.61%. Comparing base (473cc0a) to head (184e6a0).

Files Patch % Lines
deepmd/pt/model/descriptor/se_r.py 83.07% 22 Missing ⚠️
deepmd/dpmodel/descriptor/se_r.py 85.08% 17 Missing ⚠️
deepmd/pt/utils/env_mat_stat.py 69.23% 4 Missing ⚠️
deepmd/tf/descriptor/se_r.py 84.61% 4 Missing ⚠️
deepmd/pt/model/descriptor/descriptor.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            devel    #3338      +/-   ##
==========================================
+ Coverage   75.55%   75.61%   +0.06%     
==========================================
  Files         408      410       +2     
  Lines       34247    34527     +280     
  Branches     1604     1604              
==========================================
+ Hits        25876    26109     +233     
- Misses       7510     7557      +47     
  Partials      861      861              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@anyangml anyangml self-assigned this Feb 27, 2024
@anyangml anyangml requested a review from njzjz February 27, 2024 05:07
@anyangml anyangml marked this pull request as ready for review February 27, 2024 05:47
deepmd/dpmodel/descriptor/se_r.py Outdated Show resolved Hide resolved
deepmd/pt/model/descriptor/se_r.py Outdated Show resolved Hide resolved
deepmd/dpmodel/descriptor/se_r.py Outdated Show resolved Hide resolved
deepmd/dpmodel/descriptor/se_r.py Outdated Show resolved Hide resolved
@anyangml anyangml requested a review from njzjz February 27, 2024 08:48
Copy link
Member

@njzjz njzjz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, at least all tests have passed...

doc/model should be updated, saying se_r has been supported.

Copy link
Collaborator

@wanghan-iapcm wanghan-iapcm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wanghan-iapcm wanghan-iapcm added this pull request to the merge queue Feb 27, 2024
Merged via the queue into deepmodeling:devel with commit c538d04 Feb 27, 2024
48 checks passed
@njzjz njzjz mentioned this pull request Apr 2, 2024
njzjz added a commit to njzjz/deepmd-kit that referenced this pull request May 31, 2024
Closes deepmodeling#3554. I just find `se_r` never supports type embedding. It's a mistake in deepmodeling#3338.

Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
github-merge-queue bot pushed a commit that referenced this pull request Jun 2, 2024
Closes #3554. I just find `se_r` never supports type embedding. It's a
mistake in #3338.

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **Refactor**
- Improved the serialization process by removing type embedding
handling, leading to cleaner and more maintainable code.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
mtaillefumier pushed a commit to mtaillefumier/deepmd-kit that referenced this pull request Sep 18, 2024
)

Closes deepmodeling#3554. I just find `se_r` never supports type embedding. It's a
mistake in deepmodeling#3338.

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **Refactor**
- Improved the serialization process by removing type embedding
handling, leading to cleaner and more maintainable code.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants