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: apply descriptor exclude_types to env mat stat #3625

Merged
merged 8 commits into from
Apr 7, 2024

Conversation

njzjz
Copy link
Member

@njzjz njzjz commented Mar 30, 2024

No description provided.

Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Copy link

codecov bot commented Mar 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.10%. Comparing base (4c546d0) to head (a5e9377).

Additional details and impacted files
@@            Coverage Diff             @@
##            devel    #3625      +/-   ##
==========================================
- Coverage   81.71%   79.10%   -2.61%     
==========================================
  Files         503      503              
  Lines       45992    45934      -58     
  Branches     2891     2891              
==========================================
- Hits        37581    36337    -1244     
- Misses       7533     8858    +1325     
+ Partials      878      739     -139     

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

Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
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 agree with this PR, but please not that would deprecate the exclude type in the future.

deepmd/pt/utils/env_mat_stat.py Show resolved Hide resolved
deepmd/tf/descriptor/se_r.py Outdated Show resolved Hide resolved
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
@njzjz
Copy link
Member Author

njzjz commented Mar 30, 2024

would deprecate the exclude type in the future.

What's the reason for that?

@wanghan-iapcm
Copy link
Collaborator

would deprecate the exclude type in the future.

What's the reason for that?

because not all atomic model has descriptor. the dp atomic model's exclude pair is equivalent to the descriptors exclude pair.

@njzjz
Copy link
Member Author

njzjz commented Mar 31, 2024

would deprecate the exclude type in the future.

What's the reason for that?

because not all atomic model has descriptor. the dp atomic model's exclude pair is equivalent to the descriptors exclude pair.

An atomic model may not contain only one descriptor. Note that two descriptors in a DPRc model have different exclude pairs. So it may not be equivalent.

@wanghan-iapcm
Copy link
Collaborator

would deprecate the exclude type in the future.

What's the reason for that?

because not all atomic model has descriptor. the dp atomic model's exclude pair is equivalent to the descriptors exclude pair.

An atomic model may not contain only one descriptor. Note that two descriptors in a DPRc model have different exclude pairs. So it may not be equivalent.

I see in multiple descriptor scenario they are not equivalent.

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.

I notice that the excluded_types only masks the input env_mat, while still keeps the total number of atoms in excluded_types (sumn). Another way is to mask excluded_types in both numerator (sumr etc.) and denominator (sumn) . Which one should we prefer? @wanghan-iapcm @njzjz

@wanghan-iapcm
Copy link
Collaborator

I notice that the excluded_types only masks the input env_mat, while still keeps the total number of atoms in excluded_types (sumn). Another way is to mask excluded_types in both numerator (sumr etc.) and denominator (sumn) . Which one should we prefer? @wanghan-iapcm @njzjz

I would prefer mask both the numerator and the denominator, which gives consistent result as if the masked pairs do not exists.

@njzjz
Copy link
Member Author

njzjz commented Apr 1, 2024

I notice that the excluded_types only masks the input env_mat, while still keeps the total number of atoms in excluded_types (sumn). Another way is to mask excluded_types in both numerator (sumr etc.) and denominator (sumn) . Which one should we prefer? @wanghan-iapcm @njzjz

I would prefer mask both the numerator and the denominator, which gives consistent result as if the masked pairs do not exists.

They are not equivalent. When masked pairs do not exist, zeros (meaning no neighbor) will be used for the stat.

The current masking behavior has some problems. The env mat is always masked to zero, but if set_davg is false, it should be masked to davg, otherwise, there is discontinuity.

njzjz and others added 2 commits April 2, 2024 16:15
@njzjz njzjz added this to the v2.2.11 milestone Apr 6, 2024
@wanghan-iapcm wanghan-iapcm added this pull request to the merge queue Apr 7, 2024
Merged via the queue into deepmodeling:devel with commit 87d293a Apr 7, 2024
48 checks passed
njzjz added a commit to njzjz/deepmd-kit that referenced this pull request Jul 2, 2024
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
(cherry picked from commit 87d293a)
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
@njzjz njzjz mentioned this pull request Jul 2, 2024
njzjz added a commit that referenced this pull request Jul 3, 2024
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
(cherry picked from commit 87d293a)
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