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

breaking: pt: remove data stat from model init #3245

Merged
merged 9 commits into from
Feb 10, 2024

Conversation

iProzd
Copy link
Collaborator

@iProzd iProzd commented Feb 7, 2024

Restore #3233 with resolved conflicts and conversations.

This PR clean up the data stat process from model init.

Please note that this code PR is just an initial cleanup and refinement of the data stat, and a more detailed design of the data stat will be completed in the next PR:

  • independent data stat from dataloader
  • data stat support for hybrid descriptors

Copy link

codecov bot commented Feb 7, 2024

Codecov Report

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

Comparison is base (c235099) 75.19% compared to head (dd1a7a8) 75.30%.

Files Patch % Lines
deepmd/pt/model/descriptor/descriptor.py 79.16% 10 Missing ⚠️
deepmd/pt/model/descriptor/hybrid.py 9.09% 10 Missing ⚠️
deepmd/pt/model/task/ener.py 85.18% 4 Missing ⚠️
deepmd/pt/model/task/fitting.py 91.42% 3 Missing ⚠️
deepmd/dpmodel/descriptor/make_base_descriptor.py 33.33% 2 Missing ⚠️
deepmd/dpmodel/fitting/invar_fitting.py 50.00% 2 Missing ⚠️
deepmd/dpmodel/fitting/make_base_fitting.py 50.00% 2 Missing ⚠️
deepmd/pt/entrypoints/main.py 66.66% 2 Missing ⚠️
deepmd/pt/model/model/dp_atomic_model.py 94.73% 1 Missing ⚠️
deepmd/pt/model/model/model.py 0.00% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##            devel    #3245      +/-   ##
==========================================
+ Coverage   75.19%   75.30%   +0.10%     
==========================================
  Files         372      372              
  Lines       33136    33220      +84     
  Branches     1608     1608              
==========================================
+ Hits        24916    25015      +99     
+ Misses       7347     7332      -15     
  Partials      873      873              

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

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.

Are the data stats of descriptor and fitting tested by UTs?

deepmd/dpmodel/model/make_base_atomic_model.py Outdated Show resolved Hide resolved
deepmd/dpmodel/model/make_base_atomic_model.py Outdated Show resolved Hide resolved
deepmd/pt/model/descriptor/descriptor.py Show resolved Hide resolved
deepmd/pt/model/descriptor/descriptor.py Outdated Show resolved Hide resolved
deepmd/pt/model/descriptor/descriptor.py Outdated Show resolved Hide resolved
deepmd/pt/model/descriptor/descriptor.py Outdated Show resolved Hide resolved
deepmd/pt/model/task/ener.py Outdated Show resolved Hide resolved
deepmd/pt/model/task/ener.py Outdated Show resolved Hide resolved
deepmd/pt/model/model/model.py Show resolved Hide resolved
deepmd/pt/entrypoints/main.py Outdated Show resolved Hide resolved
deepmd/pt/model/descriptor/descriptor.py Show resolved Hide resolved
@iProzd
Copy link
Collaborator Author

iProzd commented Feb 8, 2024

Are the data stats of descriptor and fitting tested by UTs?

source/tests/pt/test_stat.py has the UTs to compare the consistence of data stats between tf and pt, on both descriptor and fitting but only on se_a model.

iProzd and others added 3 commits February 8, 2024 16:39
deepmd/pt/model/descriptor/hybrid.py Fixed Show resolved Hide resolved
deepmd/pt/utils/stat.py Fixed Show fixed Hide fixed
deepmd/pt/utils/stat.py Fixed Show fixed Hide fixed

def init_desc_stat(self, sumr, suma, sumn, sumr2, suma2):
def init_desc_stat(self, sumr, suma, sumn, sumr2, suma2, **kwargs):

Check warning

Code scanning / CodeQL

Signature mismatch in overriding method Warning

Overriding method 'init_desc_stat' has signature mismatch with
overridden method
.

def init_desc_stat(self, sumr, suma, sumn, sumr2, suma2):
def init_desc_stat(self, sumr, suma, sumn, sumr2, suma2, **kwargs):

Check warning

Code scanning / CodeQL

Signature mismatch in overriding method Warning

Overriding method 'init_desc_stat' has signature mismatch with
overridden method
.

def init_desc_stat(self, sumr, suma, sumn, sumr2, suma2):
def init_desc_stat(self, sumr, suma, sumn, sumr2, suma2, **kwargs):

Check warning

Code scanning / CodeQL

Signature mismatch in overriding method Warning

Overriding method 'init_desc_stat' has signature mismatch with
overridden method
.
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.

Per discussion, further refactorization will be provided by a new PR.

@wanghan-iapcm wanghan-iapcm merged commit 0e2304f into deepmodeling:devel Feb 10, 2024
46 checks passed
njzjz pushed a commit to njzjz/deepmd-kit that referenced this pull request Feb 10, 2024
Restore deepmodeling#3233 with resolved conflicts and conversations.

This PR clean up the data stat process from model init.

Please note that this code PR is just an initial cleanup and refinement
of the data stat, and a more detailed design of the data stat will be
completed in the next PR:

- independent data stat from dataloader
- data stat support for hybrid descriptors

---------

Signed-off-by: Duo <50307526+iProzd@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@njzjz njzjz mentioned this pull request Apr 2, 2024
@iProzd iProzd deleted the data_stat branch April 24, 2024 09:12
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