feat(pt/dp): add exponential switch function #4756
Conversation
|
Warning Rate limit exceeded@iProzd has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 Walkthrough""" WalkthroughA new boolean parameter Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RepFlowArgs
participant DescrptBlockRepflows
participant EnvMat
participant EnvMatUtils
User->>RepFlowArgs: Initialize (use_exp_switch)
RepFlowArgs->>DescrptBlockRepflows: Pass use_exp_switch
DescrptBlockRepflows->>EnvMat: Initialize (use_exp_switch)
EnvMat->>EnvMatUtils: Compute environment matrix (use_exp_switch)
EnvMatUtils->>EnvMatUtils: Select switch function (exp or polynomial)
EnvMatUtils-->>EnvMat: Return environment matrix
EnvMat-->>DescrptBlockRepflows: Return EnvMat instance
DescrptBlockRepflows-->>User: Descriptor ready
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
deepmd/pt/model/descriptor/env_mat.py (1)
61-61: Add more context to the parameter documentation.The current description is minimal. Consider enhancing the documentation to explain the difference between the two switch functions and when one might be preferred over the other.
- - use_exp_switch: Whether to use the exponential switch function. + - use_exp_switch: Whether to use the exponential switch function instead of the polynomial one. + The exponential switch provides a sharper transition and may be preferred for systems + where a more distinct cutoff boundary is desired.Also applies to: 74-74
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
deepmd/dpmodel/descriptor/dpa3.py(5 hunks)deepmd/dpmodel/descriptor/repflows.py(5 hunks)deepmd/dpmodel/utils/env_mat.py(5 hunks)deepmd/pt/model/descriptor/dpa3.py(1 hunks)deepmd/pt/model/descriptor/env_mat.py(6 hunks)deepmd/pt/model/descriptor/repflows.py(5 hunks)deepmd/pt/utils/preprocess.py(1 hunks)deepmd/utils/argcheck.py(2 hunks)source/tests/consistent/descriptor/test_dpa3.py(10 hunks)source/tests/universal/dpmodel/descriptor/test_descriptor.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
deepmd/pt/model/descriptor/env_mat.py (1)
deepmd/pt/utils/preprocess.py (2)
compute_exp_sw(20-29)compute_smooth_weight(9-17)
deepmd/dpmodel/descriptor/repflows.py (1)
deepmd/dpmodel/utils/env_mat.py (1)
EnvMat(101-189)
deepmd/dpmodel/utils/env_mat.py (3)
deepmd/dpmodel/array_api.py (1)
support_array_api(11-35)deepmd/pt/utils/preprocess.py (2)
compute_exp_sw(20-29)compute_smooth_weight(9-17)deepmd/pt/model/descriptor/env_mat.py (1)
_make_env_mat(11-48)
deepmd/pt/utils/preprocess.py (1)
deepmd/dpmodel/utils/env_mat.py (1)
compute_exp_sw(39-53)
⏰ Context from checks skipped due to timeout of 90000ms (29)
- GitHub Check: Test Python (6, 3.12)
- GitHub Check: Test Python (5, 3.12)
- GitHub Check: Test Python (5, 3.9)
- GitHub Check: Test Python (4, 3.12)
- GitHub Check: Test Python (4, 3.9)
- GitHub Check: Test Python (3, 3.12)
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Test Python (3, 3.9)
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Test Python (2, 3.9)
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Analyze (python)
- GitHub Check: Test Python (1, 3.12)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Test C++ (false)
- GitHub Check: Build C library (2.14, >=2.5.0rc0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Test Python (1, 3.9)
- GitHub Check: Build C++ (cuda, cuda)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Test C++ (true)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Build C++ (cpu, cpu)
🔇 Additional comments (32)
deepmd/pt/model/descriptor/dpa3.py (1)
153-153: LGTM! Addeduse_exp_switchparameter successfully.The parameter is correctly passed from
self.repflow_argsto theDescrptBlockRepflowsinitialization, maintaining consistency with other similar parameters.source/tests/universal/dpmodel/descriptor/test_descriptor.py (3)
485-485: LGTM! Addeduse_exp_switchparameter with appropriate default value.The default value of
Falseensures backward compatibility with existing code.
507-507: LGTM! Parameter propagation toRepFlowArgsis correct.The parameter is properly passed to the
RepFlowArgsconstructor.
545-545: LGTM! Test parameterization ensures coverage of both options.Including both
TrueandFalsevalues ensures the tests will verify the behavior of both switch function implementations.deepmd/pt/model/descriptor/env_mat.py (4)
6-7: LGTM! Import statement forcompute_exp_swadded correctly.The function is imported alongside the existing
compute_smooth_weightfunction.
18-18: LGTM! Parameter added to_make_env_matwith correct default value.The default value of
Falsemaintains backward compatibility.
38-42: LGTM! Conditional logic for switching between functions is clear and concise.The code cleanly uses a conditional expression to determine which switch function to use.
87-87: LGTM! Parameter propagation is handled correctly.The
use_exp_switchparameter is properly forwarded to the_make_env_matfunction.deepmd/dpmodel/descriptor/repflows.py (6)
125-134: Well-documented parameter with clear explanationsThe docstring for the
use_exp_switchparameter is comprehensive and provides:
- A clear explanation of the parameter's purpose
- The mathematical definition of the exponential switch function
- Recommended parameter values for different cutoff radii
- Guidance on selecting appropriate
rcut_smthvaluesThis documentation helps users understand how to properly configure the feature.
183-183: LGTM: Parameter added with backward compatibilityThe
use_exp_switchparameter is correctly added with a default value ofFalse, which ensures backward compatibility with existing code.
215-215: LGTM: Parameter properly stored as instance attributeThe parameter value is correctly stored as an instance attribute, following the same pattern as other parameters.
276-280: LGTM: Parameter correctly passed to EnvMat for edge calculationsThe
use_exp_switchparameter is properly propagated to the edge environment matrix.
281-286: LGTM: Parameter correctly passed to EnvMat for angle calculationsThe
use_exp_switchparameter is properly propagated to the angle environment matrix.
598-598: LGTM: Parameter included in serializationThe
use_exp_switchparameter is correctly added to the serialized dictionary, ensuring it's preserved when the object is serialized and deserialized.deepmd/utils/argcheck.py (2)
1500-1510: Well-documented new parameter for exponential switch function.The documentation for
use_exp_switchis comprehensive, clearly explaining the purpose, mathematical form, and recommended usage of the exponential switch function. It includes recommended values for the smoothing factor based on different cutoff radii.
1611-1618: Proper implementation of the exponential switch parameter.The implementation of the new
use_exp_switchparameter follows the established pattern of this codebase. The parameter is optional with a default ofFalseto maintain backward compatibility, and includes an appropriate alias "use_env_envelope".deepmd/dpmodel/descriptor/dpa3.py (5)
126-135: Well-documented parameter addition.The documentation for the
use_exp_switchparameter is comprehensive and helpful. It clearly explains what the parameter does, how the exponential switch function works, and provides guidance on choosing appropriate parameter values for different cutoff radii.
163-163: LGTM! Parameter declaration with appropriate default.The parameter is added with a default value of
False, which ensures backward compatibility with existing code.
190-190: LGTM! Proper instance attribute assignment.The parameter is correctly stored as an instance attribute.
222-222: LGTM! Parameter included in serialization.The parameter is correctly added to the serialized data dictionary.
319-319: LGTM! Parameter correctly passed to downstream component.The parameter is properly passed from
RepFlowArgstoDescrptBlockRepflows.deepmd/pt/model/descriptor/repflows.py (4)
136-145: Consistent documentation across implementations.The documentation for the
use_exp_switchparameter matches the one indpa3.py, providing a clear and consistent explanation of the feature across different parts of the codebase.
196-196: LGTM! Parameter declaration with appropriate default value.The parameter is added with a default value of
False, maintaining consistency with the implementation indpa3.py.
229-229: LGTM! Proper instance attribute assignment.The parameter is correctly stored as an instance attribute.
411-411: LGTM! Parameter correctly passed to environment matrix calculations.The
use_exp_switchparameter is passed to both edge and angle neighbor environment matrix calculations, ensuring consistent application of the selected switch function throughout the model.Also applies to: 448-448
source/tests/consistent/descriptor/test_dpa3.py (3)
68-68: LGTM! Test parameterization for new feature.The test parameterization now includes both
TrueandFalsevalues foruse_exp_switch, ensuring the feature is properly tested in both configurations.
84-84: LGTM! Parameter correctly included in test data.The
use_exp_switchparameter is properly unpacked and passed to theRepFlowArgsconstructor in the test data.Also applies to: 108-108
159-160: Appropriate test skipping for unsupported backend.Tests are correctly skipped for the PD backend when
use_exp_switchisTrue, as this feature is not yet supported in that backend.deepmd/dpmodel/utils/env_mat.py (4)
38-54: Well-implemented exponential switch function.The exponential switch function is implemented with appropriate parameter validation, follows the mathematical formula described in the documentation, and is consistent with the implementation in the PyTorch version of the code.
88-92: Clean conditional selection of switch function.The code elegantly selects between the polynomial and exponential switch functions based on the
use_exp_switchparameter, maintaining a clear structure.
107-107: LGTM! Parameter integration with EnvMat class.The
use_exp_switchparameter is properly integrated into theEnvMatclass constructor, stored as an instance attribute, and passed to the_make_env_matfunction.Also applies to: 112-112, 170-170
181-181: LGTM! Parameter included in serialization.The
use_exp_switchparameter is correctly added to the serialized data dictionary of theEnvMatclass.
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@ustc.edu.cn>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #4756 +/- ##
=======================================
Coverage 84.78% 84.78%
=======================================
Files 698 698
Lines 67709 67734 +25
Branches 3542 3540 -2
=======================================
+ Hits 57408 57430 +22
- Misses 9170 9171 +1
- Partials 1131 1133 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Duo <50307526+iProzd@users.noreply.github.com>
for more information, see https://pre-commit.ci
| a_compress_e_rate, | ||
| a_compress_use_split, | ||
| optim_update, | ||
| use_exp_switch, |
Check notice
Code scanning / CodeQL
Unused local variable Note test
Summary by CodeRabbit
New Features
Documentation
Tests
Improvements