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

[BUG] fix enable_compression in TF DescrptDPA1Compat #4114

Closed
njzjz opened this issue Sep 9, 2024 · 0 comments · Fixed by #4243
Closed

[BUG] fix enable_compression in TF DescrptDPA1Compat #4114

njzjz opened this issue Sep 9, 2024 · 0 comments · Fixed by #4243
Assignees
@iProzd
Labels
bug
Milestone
v3.0.0

Comments

@njzjz
Copy link
Member

njzjz commented Sep 9, 2024 •
edited
Loading

As discussed with @cherryWangY, the enable_compression in TF DescrptDPA1Compat (directly inherited from DescrptSeAtten) doesn't consider the suffix of the type embedding. The type embedding in the original DescrptSeAtten doesn't contain a suffix, but that in DescrptDPA1Compat does.

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

Line 2223 in b6e27e4

type_embedding = self.type_embedding.build(self.ntypes, suffix=suffix)

Users may not touch this issue, as a descriptor usually has no suffix.
@njzjz njzjz added the bug label Sep 9, 2024
@iProzd iProzd self-assigned this Sep 26, 2024
@iProzd iProzd moved this to mustfix in DeePMD-kit V3.0.0 RC Sep 26, 2024
@njzjz njzjz added this to the v3.0.0 milestone Sep 26, 2024
@coderabbitai coderabbitai bot mentioned this issue Oct 23, 2024
Merged
@iProzd iProzd linked a pull request Oct 23, 2024 that will close this issue
fix(tf): fix compress suffix in DescrptDPA1Compat #4243
Merged
github-merge-queue bot pushed a commit that referenced this issue Oct 26, 2024
@iProzd @pre-commit-ci
fix(tf): fix compress suffix in DescrptDPA1Compat (#4243)
13e247e 
Fix #4114 .
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Enhanced compression capabilities in descriptor models with new
optional parameters for improved flexibility.
- Improved serialization processes for attention layers, allowing for
better handling of scaling factors and normalization.
- Dynamic tensor name construction in utility functions to accommodate
varying suffixes.

- **Bug Fixes**
- Adjusted method parameters to ensure compatibility and functionality
with new suffix options.

- **Tests**
- Introduced a new test suite to validate the functionality of the
TensorFlow-based descriptor model, ensuring consistent output with the
updated features.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@njzjz njzjz closed this as completed Oct 26, 2024
@github-project-automation github-project-automation bot moved this from mustfix to Done in DeePMD-kit V3.0.0 RC Oct 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@iProzd iProzd

Labels
bug
Projects
Multiple backend support for DeePMD-kit
Archived in project
DeePMD-kit V3.0.0 RC
Status: Done
Milestone
v3.0.0
Development

Successfully merging a pull request may close this issue.

fix(tf): fix compress suffix in DescrptDPA1Compat iProzd/deepmd-kit
2 participants
@njzjz @iProzd