-
Notifications
You must be signed in to change notification settings - Fork 524
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
refact: the DPA1 descriptor #3696
Conversation
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.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #3696 +/- ##
==========================================
+ Coverage 81.89% 82.12% +0.23%
==========================================
Files 509 511 +2
Lines 46591 47364 +773
Branches 2952 2952
==========================================
+ Hits 38156 38899 +743
- Misses 7542 7572 +30
Partials 893 893 ☔ View full report in Codecov by Sentry. |
node_modules/.cache/prettier/.prettier-caches/9be9545d53bb64e65febe2ff48926b4145285f3a.json
Outdated
Show resolved
Hide resolved
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.
stripped type embedding should be added in a future PR.
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.
Actionable comments posted: 0
Out of diff range and nitpick comments (1)
deepmd/dpmodel/descriptor/dpa1.py (1)
279-285
: Initialization ofTypeEmbedNet
with hardcoded activation function 'Linear'. Consider making this configurable if flexibility is required.
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.
Actionable comments posted: 8
Out of diff range and nitpick comments (3)
deepmd/dpmodel/descriptor/dpa1.py (2)
206-206
: Ensure clarity on parameter usage in the constructor.It's important to clearly document which parameters are in use and which are kept for compatibility. This helps maintain clarity and prevent confusion for future developers or users of the API.
614-664
: Ensure proper handling of attention layers.When implementing complex layers like
NeighborGatedAttention
, it's crucial to ensure that each layer is handled correctly, especially in serialization and deserialization processes to maintain state consistency across sessions.deepmd/tf/descriptor/se_atten.py (1)
3-3
: Consider removing unused imports to keep the code clean and maintainable.
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.
LGTM
Some are unnecessary changes requested.
After #3696, we don't need keras anymore. Signed-off-by: Jinzhe Zeng <[email protected]>
This PR reformat DPA1 descriptor consistent for TF/PT/DP - Clean up the implementation of DPA1 descriptor in TF/PT - Add numpy implementation for DPA1 descriptor - Add `serialize` and `deserialize` for DPA1 descriptor in TF/PT/DP - Add consistence test for TF/PT/DP Now DPA1 descriptor in TF and PT&DP are consistent when: `smooth_type_embdding` == False `env_protection` == 0.0 `stripped_type_embedding` == False DPA1 descriptor in PT and DP are always consistent. Need to fix - consistence in TF: when `excluded_types` != [] and `attn_layer` > 0. --------- Signed-off-by: Duo <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
After deepmodeling#3696, we don't need keras anymore. Signed-off-by: Jinzhe Zeng <[email protected]>
This PR reformat DPA1 descriptor consistent for TF/PT/DP
serialize
anddeserialize
for DPA1 descriptor in TF/PT/DPNow DPA1 descriptor in TF and PT&DP are consistent when:
smooth_type_embdding
== Falseenv_protection
== 0.0stripped_type_embedding
== FalseDPA1 descriptor in PT and DP are always consistent.
Need to fix
excluded_types
!= [] andattn_layer
> 0.