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.

Sign up for GitHub

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

Jump to bottom

tf: handle type embedding in DescrptSeR.serialize #3554

Closed
github-actions bot opened this issue Mar 19, 2024 · 2 comments · Fixed by #3845
Closed

tf: handle type embedding in DescrptSeR.serialize #3554

github-actions bot opened this issue Mar 19, 2024 · 2 comments · Fixed by #3845
Assignees
@njzjz
Labels
branch: devel todo comment type: todo wontfix

Comments

@github-actions
Copy link

not sure how to handle type embedding - type embedding is not a model parameter,
but instead a part of the input data. Maybe the interface should be refactored...

Line: 769

deepmd-kit//deepmd/tf/descriptor/se_r.py

Lines 766 to 776 in 76371ce

raise NotImplementedError("spin is unsupported")
assert self.davg is not None
assert self.dstd is not None
# TODO: tf: handle type embedding in DescrptSeR.serialize
# not sure how to handle type embedding - type embedding is not a model parameter,
# but instead a part of the input data. Maybe the interface should be refactored...
return {
"@class": "Descriptor",
"type": "se_r",
"@version": 1,
"rcut": self.rcut,

@njzjz njzjz self-assigned this Mar 21, 2024
@njzjz
Copy link
Member

njzjz commented May 31, 2024

I just find that DescrptSeR never supports type embedding. This TODO is a mistake.

@njzjz njzjz added the wontfix label May 31, 2024
njzjz added a commit to njzjz/deepmd-kit that referenced this issue May 31, 2024
@njzjz
chore: remove type embedding TODO from se_r serialize
3f2f1a2 
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>
@njzjz njzjz mentioned this issue May 31, 2024
Merged
@njzjz njzjz linked a pull request May 31, 2024 that will close this issue
chore: remove type embedding TODO from se_r serialize #3845
Merged
github-merge-queue bot pushed a commit that referenced this issue Jun 2, 2024
@njzjz
chore: remove type embedding TODO from se_r serialize (#3845)
6378f02 
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>
@github-actions GitHub Actions
Copy link
Author

github-actions bot commented Jun 2, 2024

Closed in commit 6378f02

@github-actions github-actions bot closed this as completed Jun 2, 2024
mtaillefumier pushed a commit to mtaillefumier/deepmd-kit that referenced this issue Sep 18, 2024
@njzjz
chore: remove type embedding TODO from se_r serialize (deepmodeling#3845
26de3a4 
)

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
Assignees

@njzjz njzjz

Labels
branch: devel todo comment type: todo wontfix
Projects
No open projects
DeePMD-3.0.0 beta release
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

chore: remove type embedding TODO from se_r serialize njzjz/deepmd-kit
1 participant
@njzjz