fix(pt): fix lammps nlist sort with large sel #3993
Conversation
WalkthroughWalkthroughThe updates enhance the configurability of deep potential molecular dynamics simulations within LAMMPS by introducing the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EnerModel
participant MakeModel
participant LAMMPS
User->>EnerModel: Call forward_lower(extra_nlist_sort=True)
EnerModel->>MakeModel: Call forward_common_lower(extra_nlist_sort=True)
MakeModel->>MakeModel: Process with extra_nlist_sort
MakeModel->>EnerModel: Return results
EnerModel->>User: Return results
User->>LAMMPS: Run test suite
LAMMPS->>LAMMPS: Setup simulations with various configurations
LAMMPS->>LAMMPS: Compute energies, forces, and virials
LAMMPS->>User: Validate results with pytest assertions
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (15)
Files skipped from review due to trivial changes (1)
Files skipped from review as they are similar to previous changes (13)
Additional comments not posted (1)
Thank 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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
deepmd/pt/model/model/make_model.py (3)
243-244: Document the new parameterextra_nlist_sort.The new parameter
extra_nlist_sortshould be documented in the method's docstring for clarity.+ extra_nlist_sort + whether to forcibly sort the nlist.
383-384: Document the new parameterextra_nlist_sort.The new parameter
extra_nlist_sortshould be documented in the method's docstring for clarity.+ extra_nlist_sort + whether to forcibly sort the nlist.
408-408: Issues found with the new parameterextra_nlist_sort:
- The parameter
extra_nlist_sortis added to the_format_nlistfunction but is not utilized within the function.- There are no test cases covering the new parameter
extra_nlist_sort.Please ensure that the parameter is correctly handled within the
_format_nlistfunction and add appropriate test cases to verify its behavior.Analysis chain
Verify the impact of the new parameter
extra_nlist_sort.The new parameter
extra_nlist_sortis added with a default value ofFalse. Verify if this change impacts the behavior of the method and ensure it is tested appropriately.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the new parameter `extra_nlist_sort`. # Test: Search for the function usage. Expect: Ensure the new parameter is handled correctly. rg --type python -A 5 $'_format_nlist'Length of output: 68
Script:
#!/bin/bash # Description: Verify the impact of the new parameter `extra_nlist_sort`. # Test: Search for the function usage. Expect: Ensure the new parameter is handled correctly. rg --type py -A 5 '_format_nlist'Length of output: 1610
Script:
#!/bin/bash # Description: Verify the handling of the new parameter `extra_nlist_sort` within the `_format_nlist` function and check for related test cases. # Test: Search for the usage of `extra_nlist_sort` within the `_format_nlist` function. rg --type py 'extra_nlist_sort' -A 10 'def _format_nlist' # Test: Search for any test cases related to `_format_nlist` or `extra_nlist_sort`. rg --type py '_format_nlist|extra_nlist_sort' --glob '*test*'Length of output: 179
Script:
#!/bin/bash # Description: Verify the handling of the new parameter `extra_nlist_sort` within the `_format_nlist` function and check for related test cases. # Test: Search for the usage of `extra_nlist_sort` within the `_format_nlist` function. rg --type py -A 10 'def _format_nlist' | rg 'extra_nlist_sort' # Test: Search for any test cases related to `_format_nlist` or `extra_nlist_sort`. rg --type py '_format_nlist|extra_nlist_sort' --glob '*test*'Length of output: 206
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #3993 +/- ##
=======================================
Coverage 82.83% 82.84%
=======================================
Files 522 522
Lines 50936 51004 +68
Branches 3015 3015
=======================================
+ Hits 42195 42253 +58
- Misses 7804 7813 +9
- Partials 937 938 +1 ☔ View full report in Codecov by Sentry. |
Do you mean it only affects dpa2 but not other descriptors? |
Co-authored-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu> Signed-off-by: Duo <50307526+iProzd@users.noreply.github.com>
yes, since only dpa2 (and linear_atomic_model) uses |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (16)
source/lmp/tests/test_lammps_dpa_sel_pt.py (16)
22-34: Consider adding comments to explain variable purposes.Adding comments to explain the purpose of each variable can improve code readability and maintainability.
308-317: Consider adding comments to explain test steps.Adding comments to explain each step of the test can improve code readability and maintainability.
320-340: Consider adding comments to explain test steps.Adding comments to explain each step of the test can improve code readability and maintainability.
343-367: Consider adding comments to explain test steps.Adding comments to explain each step of the test can improve code readability and maintainability.
369-404: Consider adding comments to explain test steps.Adding comments to explain each step of the test can improve code readability and maintainability.
407-433: Consider adding comments to explain test steps.Adding comments to explain each step of the test can improve code readability and maintainability.
436-466: Consider adding comments to explain test steps.Adding comments to explain each step of the test can improve code readability and maintainability.
469-478: Consider adding comments to explain test steps.Adding comments to explain each step of the test can improve code readability and maintainability.
481-492: Consider adding comments to explain test steps.Adding comments to explain each step of the test can improve code readability and maintainability.
495-519: Consider adding comments to explain test steps.Adding comments to explain each step of the test can improve code readability and maintainability.
522-549: Consider adding comments to explain test steps.Adding comments to explain each step of the test can improve code readability and maintainability.
552-593: Consider adding comments to explain test steps.Adding comments to explain each step of the test can improve code readability and maintainability.
596-626: Consider adding comments to explain test steps.Adding comments to explain each step of the test can improve code readability and maintainability.
629-663: Consider adding comments to explain test steps.Adding comments to explain each step of the test can improve code readability and maintainability.
666-675: Consider adding comments to explain test steps.Adding comments to explain each step of the test can improve code readability and maintainability.
678-724: Consider adding comments to explain test steps.Adding comments to explain each step of the test can improve code readability and maintainability.
In this case, why do you only modify the energy model? |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
deepmd/dpmodel/model/make_model.py (2)
Ensure correct usage of
extra_nlist_sort.The
extra_nlist_sortparameter is not consistently provided in all calls toformat_nlist. Ensure that all calls toformat_nlistinclude theextra_nlist_sortparameter.
Missing
extra_nlist_sortparameter:
source/tests/pt/model/test_dp_model.py(multiple instances)source/tests/common/dpmodel/test_nlist.py(multiple instances)Correct usage:
deepmd/dpmodel/model/make_model.py(lines 342-349 and other instances)Please update the test files to include the
extra_nlist_sortparameter in all calls toformat_nlist.Analysis chain
Line range hint
319-346:
Ensure correct usage ofextra_nlist_sort.The
extra_nlist_sortparameter is correctly integrated into theformat_nlistmethod. Ensure that all calls toformat_nlistprovide the appropriate value forextra_nlist_sort.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `format_nlist` provide the appropriate value for `extra_nlist_sort`. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type python -A 5 $'format_nlist'Length of output: 67
Script:
#!/bin/bash # Description: Verify all calls to `format_nlist` provide the appropriate value for `extra_nlist_sort`. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type py -A 5 $'format_nlist'Length of output: 6211
217-222: Ensure correct usage ofextra_nlist_sort.The
extra_nlist_sortparameter is correctly integrated into thecall_lowermethod, but not all calls tocall_lowerprovide the appropriate value forextra_nlist_sort. The following instances need to be updated to include this parameter:
source/tests/common/dpmodel/test_dp_model.pysource/tests/pt/model/test_dp_model.pysource/tests/pt/model/test_ener_spin_model.pyPlease update these calls to include the
extra_nlist_sortparameter to ensure consistency and proper functionality.Analysis chain
Ensure correct usage of
extra_nlist_sort.The
extra_nlist_sortparameter is correctly integrated into thecall_lowermethod. Ensure that all calls tocall_lowerprovide the appropriate value forextra_nlist_sort.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `call_lower` provide the appropriate value for `extra_nlist_sort`. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type python -A 5 $'call_lower'Length of output: 65
Script:
#!/bin/bash # Description: Verify all calls to `call_lower` provide the appropriate value for `extra_nlist_sort`. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type py -A 5 $'call_lower'Length of output: 5318
`nlist` was not sorted and passed into `build_multiple_neighbor_list` when using dpa2 and large sel (larger than lammps max nnei). Bug fixed on several error systems but a UT needed. - [x] UT for special sel case in dpa2. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit - **New Features** - Introduced a new parameter to enhance sorting flexibility in computation methods. - Added methods to check if sorted neighbor lists are required, providing improved control over model behavior. - **Bug Fixes** - Enhanced functionality related to neighbor list formatting for improved performance across models. - **Documentation** - Updated method signatures to reflect new parameters and functionalities, clarifying usage for end-users. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Duo <50307526+iProzd@users.noreply.github.com> Co-authored-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
nlistwas not sorted and passed intobuild_multiple_neighbor_listwhen using dpa2 and large sel (larger than lammps max nnei).Bug fixed on several error systems but a UT needed.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation