fix(lmp): apply NEIGHMASK to neighbor list #4269
Conversation
Fix deepmodeling#4250. Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
📝 WalkthroughWalkthroughThe changes in this pull request introduce a new function Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 and nitpick comments (4)
source/lib/include/neighbor_list.h (1)
98-101: Consider adding input validation and documentation.While the setter implementation is correct, consider these improvements:
- Add parameter validation to prevent invalid mask values
- Enhance the documentation to explain:
- The purpose and effect of the mask
- The meaning of different bits
- Common mask values and their use cases
/** * @brief Set mask for this neighbor list. + * @param mask_ Bit mask to filter neighbor list entries + * @details The mask is applied using bitwise AND operation during neighbor list processing. + * Default value 0xFFFFFFFF means no filtering. */ void set_mask(int mask_) { mask = mask_; };source/api_c/include/c_api.h (1)
71-79: Documentation could be more detailed for the mask parameter.The function declaration and documentation follow the codebase standards. However, the documentation could be enhanced by describing the purpose and expected values of the
maskparameter.Consider expanding the documentation:
/* * @brief Set mask for a neighbor list. * - * @param nl Neighbor list. - * @param mask mask. + * @param[in] nl Neighbor list. + * @param[in] mask Bit mask to filter neighbor list entries (e.g., NEIGHMASK in LAMMPS). * @since API version 23 * **/source/api_c/src/c_api.cc (1)
45-45: Add documentation for the new function.The implementation looks good and follows the existing patterns. However, consider adding documentation to explain:
- The purpose of the mask parameter
- Expected values/range for the mask
- Any side effects of setting the mask
Add documentation above the function:
+/** + * @brief Set a mask for the neighbor list + * @param[in] nl Pointer to the neighbor list + * @param[in] mask Bit mask to filter the neighbor list (e.g., NEIGHMASK) + */ void DP_NlistSetMask(DP_Nlist* nl, int mask) { nl->nl.set_mask(mask); }source/api_c/include/deepmd.hpp (1)
614-617: LGTM! Consider enhancing the documentation.The addition of the
set_maskmethod is well-integrated and follows the codebase patterns. The implementation correctly wraps the C API call.Consider enhancing the documentation to explain:
- The purpose and effect of the mask
- The expected mask values or format
- Any relationship to LAMMPS' NEIGHMASK
/** * @brief Set mask for this neighbor list. * @param[in] mask The mask value to filter neighbor list entries. * This is typically used with LAMMPS' NEIGHMASK to properly * handle neighbor lists when bonds/angles are present. */ void set_mask(int mask) { DP_NlistSetMask(nl, mask); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- source/api_c/include/c_api.h (2 hunks)
- source/api_c/include/deepmd.hpp (1 hunks)
- source/api_c/src/c_api.cc (1 hunks)
- source/api_cc/src/common.cc (1 hunks)
- source/lib/include/neighbor_list.h (2 hunks)
- source/lmp/compute_deeptensor_atom.cpp (1 hunks)
- source/lmp/fix_dplr.cpp (1 hunks)
- source/lmp/pair_deepmd.cpp (2 hunks)
🔇 Additional comments (10)
source/lmp/compute_deeptensor_atom.cpp (2)
139-139: LGTM! Strategic addition to prevent segmentation faults.This change applies the NEIGHMASK to properly filter the neighbor list, which is crucial for preventing segmentation faults when using hybrid systems with bonds/angles and k-space calculations. The placement right before tensor computation ensures proper neighbor handling.
139-139: Verify consistent NEIGHMASK usage across the codebase.Let's ensure this mask is consistently applied across all neighbor list implementations.
✅ Verification successful
Based on the search results, I can see that NEIGHMASK is consistently used with set_mask() across the codebase in the following files:
- source/lmp/fix_dplr.cpp
- source/lmp/compute_deeptensor_atom.cpp
- source/lmp/pair_deepmd.cpp
The usage pattern is consistent - NEIGHMASK is set on InputNlist objects before they are used for neighbor list operations. This is true for both regular and extended neighbor lists.
NEIGHMASK is consistently applied across all neighbor list implementations in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent NEIGHMASK usage in neighbor list handling # Search for neighbor list mask usage echo "=== Checking NEIGHMASK usage ===" rg -A 2 "set_mask.*NEIGHMASK" # Search for InputNlist instantiations to verify mask setting echo "=== Checking InputNlist usage ===" rg -A 5 "InputNlist.*\("Length of output: 73575
source/lib/include/neighbor_list.h (2)
45-46: LGTM: Mask initialization maintains backward compatibility.The default value of
0xFFFFFFFFensures all bits are set, meaning no filtering by default. This maintains backward compatibility with existing code while allowing for selective filtering when needed.
Line range hint
45-101: Verify the impact on memory layout and ABI compatibility.The addition of a new member variable could affect the memory layout of the
InputNliststruct. Since this struct is part of the public API and used in LAMMPS integration, we should verify:
- The impact on existing code that might depend on the struct's memory layout
- Whether this change requires coordination with LAMMPS maintainers
✅ Verification successful
The addition of the mask member variable is safe to proceed
Based on the analysis of the codebase:
- The
InputNliststruct is used primarily for passing neighbor list information and doesn't involve any binary serialization, packing, or strict memory layout dependencies.- In LAMMPS integration (pair_deepmd.cpp), the struct is used through its constructors and member functions, not through direct memory operations.
- The test files show the struct is used for its intended purpose of managing neighbor lists, with no reliance on its memory layout.
- The mask member is properly initialized in all constructors and accessed only through the
set_maskmethod, maintaining encapsulation.The change is safe as it:
- Doesn't break the ABI as there's no evidence of direct memory operations or binary serialization
- Maintains backward compatibility through proper initialization in all constructors
- Is used consistently through member functions rather than direct memory access
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for direct struct memory operations that might be affected # Look for potential memory operations on InputNlist rg -l "memcpy.*InputNlist|malloc.*InputNlist|realloc.*InputNlist" # Look for size-dependent operations rg -l "sizeof.*InputNlist" # Look for serialization/deserialization code rg -l "serialize.*InputNlist|deserialize.*InputNlist"Length of output: 152
Script:
#!/bin/bash # Let's try a broader search to understand the usage of InputNlist # Look for files that use InputNlist rg -l "InputNlist" # Look for potential inheritance or composition ast-grep --pattern 'class $_ : $$$public InputNlist$$$' # Check for any pointer or reference usage of InputNlist rg "InputNlist\s*[*&]" -l # Look for any test files that might reveal usage patterns fd "test.*\.(cpp|h|cc)" --exec rg -l "InputNlist" {}Length of output: 5430
Script:
#!/bin/bash # Let's examine how InputNlist is used in LAMMPS integration and key files # Check LAMMPS integration files for struct usage patterns rg -A 5 -B 5 "InputNlist" source/lmp/pair_deepmd.cpp source/lmp/pair_deepmd.h # Look at the test files to understand memory handling rg -A 5 -B 5 "InputNlist" source/lib/tests/test_neighbor_list.cc # Check if there are any binary data handling or memory layout dependencies rg "pack|unpack|binary|struct.*layout" source/lib/include/neighbor_list.hLength of output: 5914
source/api_cc/src/common.cc (1)
244-246: LGTM! The neighbor list masking implementation looks correct.The addition of the bitwise AND operation with
inlist.maskproperly filters the neighbor list entries, which is crucial for:
- Preventing segmentation faults by masking out invalid neighbor indices
- Supporting LAMMPS' NEIGHMASK functionality for proper handling of bond/angle information
This change effectively addresses the segmentation fault issue mentioned in PR #4250 that occurs when running LAMMPS with a combination of the DeepMD model, k-space, and bond/angle information.
source/lmp/pair_deepmd.cpp (2)
568-568: LGTM! Important fix for neighbor list filtering.The addition of
set_mask(NEIGHMASK)ensures proper filtering of the neighbor list to handle non-bonded interactions correctly. This is crucial for fixing the segmentation fault when using k-space with bonds/angles.
578-578: LGTM! Consistent neighbor list filtering for spin systems.The addition of
set_mask(NEIGHMASK)to the extended neighbor list ensures consistent filtering behavior for systems with spin interactions. This maintains proper handling of non-bonded interactions across all system types.source/api_c/include/c_api.h (1)
15-15: LGTM: API version increment is appropriate.The version bump from 22 to 23 correctly follows API versioning practices as we're adding a new function
DP_NlistSetMask.source/api_c/include/deepmd.hpp (1)
614-617: Verify the integration of the new mask functionality.Let's verify the complete integration of this change:
✅ Verification successful
The
set_maskfunctionality is properly integrated and used consistentlyThe verification shows:
- The
set_maskmethod is consistently used in LAMMPS integration files (fix_dplr.cpp,pair_deepmd.cpp,compute_deeptensor_atom.cpp)- All usages correctly pass the
NEIGHMASKparameter- The implementation chain is complete:
- C++ API in
neighbor_list.h- C API wrapper in
c_api.cc- High-level wrapper in
deepmd.hpp🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of the new set_mask method and API version changes # Test 1: Find all potential callers of set_mask echo "=== Potential callers of set_mask ===" rg "set_mask" --type cpp # Test 2: Verify API version increment echo -e "\n=== API version changes ===" rg "API_VERSION|VERSION_NUMBER" --type cpp # Test 3: Check for any NEIGHMASK usage echo -e "\n=== NEIGHMASK usage ===" rg "NEIGHMASK" --type cppLength of output: 3089
source/lmp/fix_dplr.cpp (1)
466-466: Addition of neighbor list mask setting is appropriateThe line
lmp_list.set_mask(NEIGHMASK);correctly sets the neighbor list mask, ensuring proper neighbor list configuration and resolving the segmentation fault issue described in issue #4250.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #4269 +/- ##
=======================================
Coverage 84.34% 84.34%
=======================================
Files 549 549
Lines 51464 51472 +8
Branches 3047 3055 +8
=======================================
+ Hits 43406 43414 +8
- Misses 7096 7098 +2
+ Partials 962 960 -2 ☔ View full report in Codecov by Sentry. |
|
@njzjz please cite the source of the solution in the issue description . thanks! |
Fix #4250.
See lammps/lammps#581 (comment) for an explanation of
NEIGHMASK.Summary by CodeRabbit
Release Notes
New Features
InputNliststructure for setting the mask.ComputeDeeptensorAtomandFixDPLRclasses to utilize neighbor list masks in computations.Bug Fixes
FixDPLRclass with enhanced error handling.Documentation