-
Notifications
You must be signed in to change notification settings - Fork 526
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
feat: apply descriptor exclude_types to env mat stat #3625
Conversation
Signed-off-by: Jinzhe Zeng <[email protected]>
Signed-off-by: Jinzhe Zeng <[email protected]>
Signed-off-by: Jinzhe Zeng <[email protected]>
Signed-off-by: Jinzhe Zeng <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
Signed-off-by: Jinzhe Zeng <[email protected]>
There was a problem hiding this 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.
Signed-off-by: Jinzhe Zeng <[email protected]>
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. |
There was a problem hiding this 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
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 |
Signed-off-by: Jinzhe Zeng <[email protected]>
for more information, see https://pre-commit.ci
Signed-off-by: Jinzhe Zeng <[email protected]> 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 <[email protected]>
Signed-off-by: Jinzhe Zeng <[email protected]> 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 <[email protected]>
No description provided.