-
Notifications
You must be signed in to change notification settings - Fork 528
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): copy nloc atoms from neighbor list #4459
Conversation
Prevent that the size of neighbor list is larger than nloc. Signed-off-by: Jinzhe Zeng <[email protected]>
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.
Copilot wasn't able to review any files in this pull request.
Files not reviewed (5)
- source/api_cc/include/common.h: Language not supported
- source/api_cc/src/DeepPotJAX.cc: Language not supported
- source/api_cc/src/DeepPotPT.cc: Language not supported
- source/api_cc/src/DeepSpinPT.cc: Language not supported
- source/api_cc/src/common.cc: Language not supported
📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to several classes and methods related to neighbor list data management in the codebase. The Changes
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 (
|
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: 1
🧹 Outside diff range and nitpick comments (1)
source/api_cc/include/common.h (1)
36-42
: Clarify documentation forcopy_from_nlist
methodThe updated documentation effectively explains the new
natoms
parameter. To enhance readability, consider combining the parameter description into a single line.Apply this diff to adjust the documentation:
/** * @brief Copy the neighbor list from an InputNlist. - * @param[in] inlist The input neighbor list. - * @param[in] natoms The number of atoms to copy. If natoms is -1, copy all - * atoms. + * @param[in] inlist The input neighbor list. + * @param[in] natoms The number of atoms to copy. If natoms is -1, copy all atoms. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
source/api_cc/include/common.h
(1 hunks)source/api_cc/src/DeepPotJAX.cc
(1 hunks)source/api_cc/src/DeepPotPT.cc
(1 hunks)source/api_cc/src/DeepSpinPT.cc
(1 hunks)source/api_cc/src/common.cc
(1 hunks)
🔇 Additional comments (3)
source/api_cc/src/DeepPotPT.cc (1)
172-172
: Correct usage of updated copy_from_nlist
method
The addition of nall - nghost
as the second parameter to copy_from_nlist
aligns with the updated method signature and ensures that only the relevant atoms are copied. This change enhances the accuracy of neighbor list management.
source/api_cc/src/DeepPotJAX.cc (1)
569-569
: Proper implementation of copy_from_nlist
with new parameter
Including nall - nghost
as the second argument in nlist_data.copy_from_nlist(lmp_list, nall - nghost);
correctly adapts the method call to the updated signature. This ensures appropriate handling of neighbor list data based on the number of atoms.
source/api_cc/src/DeepSpinPT.cc (1)
180-180
: Consistent application of updated copy_from_nlist
method
By passing nall - nghost
to copy_from_nlist
, the code accurately reflects the updated method signature and ensures that neighbor list data is correctly managed for the local atoms.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## devel #4459 +/- ##
=======================================
Coverage 83.75% 83.75%
=======================================
Files 667 667
Lines 61518 61517 -1
Branches 3486 3486
=======================================
Hits 51526 51526
+ Misses 8866 8865 -1
Partials 1126 1126 ☔ View full report in Codecov by Sentry. |
Prevent that the size of the neighbor list is larger than nloc.
Summary by CodeRabbit
New Features
natoms
parameter.compute
methods across multiple classes.Bug Fixes
translate_error
method for better clarity on exceptions.Documentation
Style