-
Notifications
You must be signed in to change notification settings - Fork 519
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
fix(pt/dp): share params of repinit_three_body
#4139
Conversation
WalkthroughWalkthroughThe changes in this pull request enhance the Changes
Possibly related PRs
Suggested labels
Suggested reviewersThank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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
Outside diff range and nitpick comments (1)
deepmd/pt/model/descriptor/dpa2.py (1)
513-513
: Rename unused loop variable.As per the Ruff suggestion, please rename the unused loop variable
ii
to_ii
to indicate that it is intentionally unused.-for ii, descrpt in enumerate(descrpt_list): +for _ii, descrpt in enumerate(descrpt_list):Tools
Ruff
513-513: Loop control variable
ii
not used within loop bodyRename unused
ii
to_ii
(B007)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- deepmd/dpmodel/descriptor/dpa2.py (1 hunks)
- deepmd/pt/model/descriptor/dpa2.py (4 hunks)
- source/tests/pt/model/test_permutation.py (1 hunks)
- source/tests/pt/test_multitask.py (2 hunks)
Additional context used
Ruff
deepmd/pt/model/descriptor/dpa2.py
513-513: Loop control variable
ii
not used within loop bodyRename unused
ii
to_ii
(B007)
Additional comments not posted (6)
source/tests/pt/test_multitask.py (1)
304-341
: LGTM!The new test class
TestMultiTaskDPA2Tebd
is well-structured and correctly sets up the necessary configuration and paths for testing themodel_dpa2tebd
configuration. ThesetUp
andtearDown
methods are properly implemented, and there are no apparent issues with the logic or syntax.This addition enhances the testing framework by providing a dedicated test case for the
model_dpa2tebd
configuration, thereby expanding the coverage of the multitask training functionality.source/tests/pt/model/test_permutation.py (1)
167-217
: Verify the impact of the key changes inmodel_dpa2tebd
.The
model_dpa2tebd
configuration introduces several changes compared tomodel_dpa2
:
- Addition of 3-body interactions in the descriptor, which could improve accuracy for certain systems but increases computational cost.
- Changes in the "repformer" section, such as reducing the number of layers, disabling certain attention mechanisms, and introducing new configuration parameters.
- A simpler fitting network with one less hidden layer.
Please verify the model's performance with these changes and ensure that the trade-offs align with the desired objectives.
To verify the impact of the changes, you can run the following script:
The script searches for evidence that:
- Both models are being tested and compared in the test files.
- Both models are being trained and evaluated on the same dataset.
- There is some documentation or comments discussing the changes and their expected impact.
If the script finds relevant results, it would indicate that the impact of the changes has been considered and verified. If not, it suggests that more testing and documentation may be needed.
deepmd/pt/model/descriptor/dpa2.py (2)
392-395
: LGTM!The changes to conditionally share parameters of
repinit_three_body
based onuse_three_body
look good. This ensures consistency when the three-body interaction is enabled.Also applies to: 405-408
510-513
: Looks good!The changes to include
repinit_three_body
incompute_input_stats
,set_stat_mean_and_stddev
,get_stat_mean_and_stddev
andserialize
whenuse_three_body
is enabled are correct and consistent.Also applies to: 522-525, 531-539
Tools
Ruff
513-513: Loop control variable
ii
not used within loop bodyRename unused
ii
to_ii
(B007)
deepmd/dpmodel/descriptor/dpa2.py (2)
736-739
: LGTM!The change correctly updates the mean and stddev for the
self.repinit_three_body
descriptor whenself.use_three_body
is True. This ensures that the statistics are properly maintained for all relevant descriptors.
745-753
: LGTM!The change correctly includes the mean and stddev of the
self.repinit_three_body
descriptor in the returned lists whenself.use_three_body
is True. This ensures that the retrieved statistics are comprehensive and consistent with the enabled descriptors.
Fix #4137. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced descriptor handling to support three-body interactions, improving model flexibility. - Introduced a new model configuration, `model_dpa2tebd`, with advanced parameters for better performance. - **Tests** - Added a new test class, `TestMultiTaskDPA2Tebd`, to expand testing coverage for the new model configuration. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Fix #4137.
Summary by CodeRabbit
New Features
model_dpa2tebd
, with advanced parameters for better performance.Tests
TestMultiTaskDPA2Tebd
, to expand testing coverage for the new model configuration.