-
Notifications
You must be signed in to change notification settings - Fork 520
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(cc): add atomic
argument to DeepPotBase::computew
#3996
Conversation
See deepmodeling#3969 for the background. Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
WalkthroughWalkthroughRecent changes to the Changes
Sequence Diagram(s)DeepPotPT: Conditional Execution Based on
|
DeepPotBase
atomic
argument to DeepPotBase.computew
atomic
argument to DeepPotBase.computew
atomic
argument to DeepPotBase::computew
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, codebase verification and nitpick comments (7)
source/api_cc/include/DeepPotTF.h (3)
74-76
: Update allcompute
method calls to include theatomic
parameter.The
atomic
parameter has been added to the method signature, but none of the calls to this method have been updated accordingly. This could lead to compilation errors or incorrect behavior. Please update the following locations to include theatomic
parameter:
source/api_cc/src/DeepTensorTF.cc
source/api_cc/src/DeepTensor.cc
source/api_cc/src/DeepPotTF.cc
source/api_cc/src/DataModifier.cc
source/api_cc/src/DataModifierTF.cc
source/api_cc/src/DeepPotPT.cc
source/api_cc/src/DeepPot.cc
Analysis chain
Verify the new parameter integration.
The
atomic
parameter has been added to the method signature. Ensure all calls to this method are updated accordingly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `compute` method are updated with the new `atomic` parameter. # Test: Search for the method calls. Expect: Only occurances of the new signature. rg --type cpp -A 5 $'compute<'Length of output: 32052
153-155
: Integration of theatomic
parameter is incomplete.The
atomic
parameter has been added to the method signature ofcompute_mixed_type
, but none of the calls to this method have been updated to include this new parameter. Please ensure that all calls tocompute_mixed_type
are updated to include theatomic
parameter.
- Files to update:
source/api_cc/src/DeepPot.cc
source/api_cc/src/DeepPotTF.cc
Analysis chain
Verify the new parameter integration.
The
atomic
parameter has been added to the method signature. Ensure all calls to this method are updated accordingly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `compute_mixed_type` method are updated with the new `atomic` parameter. # Test: Search for the method calls. Expect: Only occurances of the new signature. rg --type cpp -A 5 $'compute_mixed_type<'Length of output: 4943
Script:
#!/bin/bash # Description: Verify all calls to `compute_mixed_type` method are updated with the new `atomic` parameter. # Search for the method calls to check if the `atomic` parameter is included. rg --type cpp 'compute_mixed_type<' -A 10Length of output: 8729
115-117
: Verify the new parameter integration.The
atomic
parameter has not been integrated into thecompute
method signatures inDeepPotTF.cc
. Please ensure all instances of thecompute
method are updated to include theatomic
parameter.
- File:
source/api_cc/src/DeepPotTF.cc
- Method Signatures:
template void DeepPotTF::compute<double, ENERGYTYPE>(...)
template void DeepPotTF::compute<float, ENERGYTYPE>(...)
template void DeepPotTF::compute<double, std::vector<ENERGYTYPE>>(...)
template void DeepPotTF::compute<float, std::vector<ENERGYTYPE>>(...)
Analysis chain
Verify the new parameter integration.
The
atomic
parameter has been added to the method signature. Ensure all calls to this method are updated accordingly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `compute` method with additional parameters are updated with the new `atomic` parameter. # Test: Search for the method calls. Expect: Only occurances of the new signature. rg --type cpp -A 5 $'compute<'Length of output: 32052
Script:
#!/bin/bash # Description: Verify all calls to `compute` method include the new `atomic` parameter. # Test: Search for the method signatures. Expect: Presence of the `atomic` parameter in the method signature. rg --type cpp -A 10 'template void DeepPotTF::compute<' | grep -A 10 'compute<' rg --type cpp -A 10 'template void DeepTensorTF::compute<' | grep -A 10 'compute<' rg --type cpp -A 10 'template void DeepPotPT::compute<' | grep -A 10 'compute<' rg --type cpp -A 10 'template void DipoleChargeModifierTF::compute<' | grep -A 10 'compute<' rg --type cpp -A 10 'template void DipoleChargeModifier::compute<' | grep -A 10 'compute<' rg --type cpp -A 10 'template void DeepPot::compute<' | grep -A 10 'compute<' rg --type cpp -A 10 'template void DeepPotModelDevi::compute<' | grep -A 10 'compute<'Length of output: 41762
Script:
#!/bin/bash # Description: Verify all calls to `compute` method include the new `atomic` parameter. # Test: Search for the method signatures in the context of the `DeepPotTF.h` file. Expect: Presence of the `atomic` parameter in the method signature. rg --type cpp -A 10 'compute<' source/api_cc/include/DeepPotTF.hLength of output: 66
Script:
#!/bin/bash # Description: Verify all calls to `compute` method include the new `atomic` parameter. # Test: Search for the method signatures in the relevant implementation files. Expect: Presence of the `atomic` parameter in the method signature. rg --type cpp -A 10 'compute<' source/api_cc/src/DeepPotTF.ccLength of output: 3396
source/api_cc/include/DeepPotPT.h (3)
150-151
: Update method calls to include the newatomic
parameter.The
atomic
parameter has been added to thecompute_mixed_type
method signature, but the method calls have not been updated accordingly. Please ensure all calls to this method are updated to include the newatomic
parameter.
- Instances found in
source/api_cc/src/DeepPotTF.cc
- Instances found in
source/api_cc/src/DeepPot.cc
Analysis chain
Verify the new parameter integration.
The
atomic
parameter has been added to the method signature. Ensure all calls to this method are updated accordingly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `compute_mixed_type` method are updated with the new `atomic` parameter. # Test: Search for the method calls. Expect: Only occurances of the new signature. rg --type cpp -A 5 $'compute_mixed_type<'Length of output: 4943
Script:
#!/bin/bash # Description: Verify all calls to `compute_mixed_type` method are updated with the new `atomic` parameter. # Test: Search for the method calls. Expect: Only occurrences of the new signature with `atomic` parameter. rg --type cpp -A 10 'compute_mixed_type<'Length of output: 8729
115-117
: Update allcompute
method calls to include the newatomic
parameter.The
atomic
parameter has been added to thecompute
method signature, but the method calls have not been updated to include this new parameter. Please ensure all calls to this method are updated accordingly.
- Files to update:
source/api_cc/src/DeepTensorTF.cc
source/api_cc/src/DeepTensor.cc
source/api_cc/src/DataModifierTF.cc
source/api_cc/src/DataModifier.cc
source/api_cc/src/DeepPotTF.cc
source/api_cc/src/DeepPot.cc
Analysis chain
Verify the new parameter integration.
The
atomic
parameter has been added to the method signature. Ensure all calls to this method are updated accordingly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `compute` method with additional parameters are updated with the new `atomic` parameter. # Test: Search for the method calls. Expect: Only occurances of the new signature. rg --type cpp -A 5 $'compute<'Length of output: 32052
Script:
#!/bin/bash # Description: Verify all calls to `compute` method with additional parameters are updated with the new `atomic` parameter. # Test: Search for the method calls and include more context lines to verify the presence of the `atomic` parameter. rg --type cpp -A 10 'compute<'Length of output: 51990
74-76
: Update method calls to include new parameter.The
atomic
parameter has been added to thecompute
method inDeepPotPT.h
, but the calls to this method in the source files have not been updated accordingly. Please update all calls to this method to include theatomic
parameter.
- Files to update:
source/api_cc/src/DeepTensorTF.cc
source/api_cc/src/DeepPotTF.cc
source/api_cc/src/DataModifierTF.cc
source/api_cc/src/DeepPotPT.cc
source/api_cc/src/DeepPot.cc
source/api_cc/src/DataModifier.cc
source/api_cc/src/DeepTensor.cc
Analysis chain
Verify the new parameter integration.
The
atomic
parameter has been added to the method signature. Ensure all calls to this method are updated accordingly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `compute` method are updated with the new `atomic` parameter. # Test: Search for the method calls. Expect: Only occurances of the new signature. rg --type cpp -A 5 $'compute<'Length of output: 32052
Script:
#!/bin/bash # Description: Verify the method declaration of `compute` in `DeepPotPT.h` to check for the `atomic` parameter. # Test: Search for the method declaration. Expect: The method declaration includes the `atomic` parameter. rg --type cpp -A 5 'compute' source/api_cc/include/DeepPotPT.hLength of output: 3627
source/api_cc/src/DeepPotTF.cc (1)
835-836
: Ensure proper handling of theatomic
parameter.The
atomic
parameter has been added to the function signature ofcompute_mixed_type
. However, not all calls to this function include theatomic
parameter. Please update the following instances to pass the correct value foratomic
:
source/api_cc/src/DeepPotTF.cc
source/api_cc/src/DeepPot.cc
source/api_cc/tests/test_mixed_type.cc
source/api_c/tests/test_mixed_type.cc
source/api_c/src/c_api.cc
source/api_c/include/deepmd.hpp
Analysis chain
Ensure proper handling of the
atomic
parameter.The
atomic
parameter is added to the function signature. Ensure that all calls to this function pass the correct value foratomic
.
Ensure proper handling of the
atomic
parameter.The
atomic
parameter has been added to the function signature ofcompute_mixed_type
. Ensure that all calls to this function pass the correct value foratomic
.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `compute_mixed_type` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type cpp -A 5 $'compute_mixed_type'Length of output: 15771
Script:
#!/bin/bash # Description: Verify all function calls to `compute_mixed_type` include the `atomic` parameter. # Test: Search for the function usage and check if the `atomic` parameter is included. Expect: Only occurrences with the `atomic` parameter. rg --type cpp 'compute_mixed_type' -A 0 | grep -v 'atomic'Length of output: 3126
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #3996 +/- ##
==========================================
+ Coverage 82.83% 82.90% +0.07%
==========================================
Files 522 522
Lines 50889 50888 -1
Branches 3015 3022 +7
==========================================
+ Hits 42154 42189 +35
+ Misses 7798 7754 -44
- Partials 937 945 +8 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
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
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.
It seems that the atomic option is only in C++ and not in C, while the default lammps compile option is C. We should better support this option in C.
This may be completed in a separated PR. |
…ng#3996) See deepmodeling#3969 for the background. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced an 'atomic' parameter in various compute functions to enable atomic energy and virial calculations, providing more granular control over computations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
See #3969 for the background.
Summary by CodeRabbit