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

support TF se_e2_a serialization; add a common test fixture to compare TF, PT, and DP models #3263

Merged
merged 17 commits into from
Feb 14, 2024
Merged

support TF se_e2_a serialization; add a common test fixture to compare TF, PT, and DP models #3263

merged 17 commits into from
Feb 14, 2024

Conversation

njzjz
Copy link
Member

@njzjz njzjz commented Feb 12, 2024

Some codes are split from #2987.

@njzjz njzjz changed the title support TF se_e2_a serialization; add a test fixture to compare TF, PT, and DP models support TF se_e2_a serialization; add a common test fixture to compare TF, PT, and DP models Feb 12, 2024
@njzjz
fix py38 compatibility
5f6b64d 
Signed-off-by: Jinzhe Zeng <[email protected]>
@codecov Codecov
Copy link

codecov bot commented Feb 12, 2024 •
edited
Loading

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (6018e3c) 75.27% compared to head (e88f8fe) 75.32%.

Files Patch % Lines
deepmd/tf/descriptor/se_a.py 79.31% 6 Missing ⚠️
deepmd/tf/descriptor/descriptor.py 42.85% 4 Missing ⚠️
deepmd/tf/descriptor/se.py 94.11% 4 Missing ⚠️
deepmd/dpmodel/descriptor/se_e2_a.py 50.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##            devel    #3263      +/-   ##
==========================================
+ Coverage   75.27%   75.32%   +0.04%     
==========================================
  Files         373      373              
  Lines       33252    33362     +110     
  Branches     1604     1604              
==========================================
+ Hits        25031    25130      +99     
- Misses       7350     7361      +11     
  Partials      871      871

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

iProzd
iProzd reviewed Feb 12, 2024
View reviewed changes
Copy link
Collaborator

@iProzd iProzd left a comment

Choose a reason for hiding this comment

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

Should the status of pt, tf, and dp in testing be considered equivalent? Should we set self.skip_tf, self.skip_pt, and self.skip_dp all? In future model implementations, there might be parts that are not available in tf as well.

source/tests/consistent/common.py Outdated Show resolved Hide resolved
iProzd
iProzd reviewed Feb 12, 2024
View reviewed changes
source/tests/consistent/test_se_e2_a.py Outdated Show resolved Hide resolved
source/tests/consistent/test_se_e2_a.py Outdated Show resolved Hide resolved
wanghan-iapcm
wanghan-iapcm reviewed Feb 12, 2024
View reviewed changes
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.

I am not sure if the current design is a good idea, because to test the implementation of pt, one have to install the tf. suppose we have 5 backends, the env would be very heavy for the developers.

Ideally, the dp backend can be tested without any deep learning platforms, while any implementation, e.g. pt, can be tested without any other backend.

source/tests/consistent/test_se_e2_a.py Outdated Show resolved Hide resolved
source/tests/consistent/common.py Outdated Show resolved Hide resolved
njzjz added 3 commits February 12, 2024 10:30
@njzjz
redesign test suite
f56695d 
Signed-off-by: Jinzhe Zeng <[email protected]>
@njzjz
refactor
ef3fb13 
Signed-off-by: Jinzhe Zeng <[email protected]>
@njzjz
make common descriptor test fixture
a3b7e0f 
Signed-off-by: Jinzhe Zeng <[email protected]>
wanghan-iapcm
wanghan-iapcm reviewed Feb 13, 2024
View reviewed changes
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.

would it be possible to loop over a list of possible model parameters like

deepmd-kit/source/tests/pt/model/test_se_e2_a.py

Lines 43 to 45 in 392b9e0

for idt, prec in itertools.product(
[False, True],
["float64", "float32"],

source/tests/consistent/common.py Outdated Show resolved Hide resolved
source/tests/consistent/common.py Outdated Show resolved Hide resolved
njzjz and others added 9 commits February 13, 2024 04:35
@njzjz
Merge branch 'devel' into serialize-se-a
aa58c94
@njzjz
adjust directory structure
efa10fc 
Signed-off-by: Jinzhe Zeng <[email protected]>
@njzjz
add doc
115bfe7 
Signed-off-by: Jinzhe Zeng <[email protected]>
@njzjz
add a decorator to parameterize the class
e804b6a 
Signed-off-by: Jinzhe Zeng <[email protected]>
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
e192853 
for more information, see https://pre-commit.ci
@njzjz
minor fixes
fc6f48e 
Signed-off-by: Jinzhe Zeng <[email protected]>
@njzjz
Merge branch 'serialize-se-a' of https://github.com/njzjz/deepmd-kit …
15d4b53 
…into serialize-se-a
@njzjz
Revert "[pre-commit.ci] auto fixes from pre-commit.com hooks"
7441e4c 
This reverts commit e192853.
@njzjz
revert changes to ruff configs; should be addressed in another PR
e88f8fe 
Signed-off-by: Jinzhe Zeng <[email protected]>
@njzjz njzjz requested a review from wanghan-iapcm February 13, 2024 10:46
wanghan-iapcm
wanghan-iapcm approved these changes Feb 13, 2024
View reviewed changes
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.

I like the design of decorator parameterized very much.

@wanghan-iapcm wanghan-iapcm merged commit 930bc1a into deepmodeling:devel Feb 14, 2024
46 checks passed
@njzjz njzjz mentioned this pull request Apr 2, 2024
Closed
njzjz added a commit to njzjz/deepmd-kit that referenced this pull request Jul 5, 2024
@njzjz
[r2] fix: move resnet_dt checking from graph to tabulate
897376a 
Fix deepmodeling#3950. Backport a part of deepmodeling#3263 to r2 (the whole of deepmodeling#3263 is not likely to be backported).

Signed-off-by: Jinzhe Zeng <[email protected]>
@njzjz njzjz mentioned this pull request Jul 5, 2024
Merged
wanghan-iapcm pushed a commit that referenced this pull request Jul 6, 2024
@njzjz
[r2] fix: move resnet_dt checking from graph to tabulate (#3952)
6dafa50 
Fix #3950. Backport a part of #3263 to r2 (the whole of #3263 is not
likely to be backported).

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

## Summary by CodeRabbit

- **New Features**
- Introduced new checks to ensure only weight and bias matrices are
supported during tabulation operations.

- **Refactor**
- Removed redundant loop enforcing constraints on node keys related to
weight and bias matrices to streamline operations.

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

Signed-off-by: Jinzhe Zeng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wanghan-iapcm wanghan-iapcm wanghan-iapcm approved these changes

@iProzd iProzd iProzd approved these changes

Assignees
No one assigned
Labels
Python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@njzjz @iProzd @wanghan-iapcm