chore(lmp): add LAMMPS DPA-2 nopbc tests #4220
Conversation
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
📝 WalkthroughWalkthroughThe changes introduce a new command-line argument Changes
Sequence Diagram(s)sequenceDiagram participant User participant Script participant LAMMPS User->>Script: Run with --nopbc Script->>LAMMPS: Set boundary conditions to "f f f" Script->>LAMMPS: Execute simulation LAMMPS-->>Script: Return results Script-->>User: Display results 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: 3
🧹 Outside diff range and nitpick comments (3)
source/lmp/tests/run_mpi_pair_deepmd.py (2)
42-45: LGTM: Boundary conditions set correctly based on--nopbcargument.The implementation correctly uses the new
--nopbcargument to set the boundary conditions. This change allows for testing both periodic and non-periodic boundary conditions, which is crucial for verifying the issues mentioned in PR #4167.Consider adding a comment explaining the meaning of "f f f" and "p p p" for improved clarity:
if args.nopbc: lammps.boundary("f f f") # Set free boundaries in all directions else: lammps.boundary("p p p") # Set periodic boundaries in all directions
Line range hint
1-67: Consider adding error handling and a test case for MPI-related issues.While the changes implemented are correct, the PR description mentions segmentation faults when using MPI. To address this:
- Consider adding error handling around the MPI calls to catch and report any segmentation faults or other errors that might occur during execution.
- It might be beneficial to add a test case that attempts to reproduce the segmentation fault mentioned in the PR description. This could help in identifying and resolving the issue.
Here's a simple example of how you might add some basic error handling:
try: # Your existing MPI code here ... MPI.Finalize() except Exception as e: print(f"Error occurred during MPI execution: {e}") # You might want to log this error or handle it in a way that's appropriate for your testing framework raiseWould you like assistance in implementing more robust error handling or creating a test case to reproduce the segmentation fault?
source/lmp/tests/test_lammps_dpa_pt_nopbc.py (1)
685-691: Remove commented-out code to enhance readabilityThe stack trace comments within the
if balance_args == []:block are unnecessary and can clutter the code. If this information is important for debugging or historical context, consider moving it to a dedicated documentation file or including it in the test's docstring.Apply this diff to clean up the code:
if balance_args == []: - # python:5331 terminated with signal 11 at PC=7f3e940e3806 SP=7ffd5787edc0. Backtrace: - # /home/runner/work/deepmd-kit/deepmd-kit/dp_test/lib/libdeepmd_op_pt.so(+0x95806)[0x7f3e940e3806] - # /home/runner/work/deepmd-kit/deepmd-kit/dp_test/lib/libdeepmd_op_pt.so(+0x8f76e)[0x7f3e940dd76e] - # /home/runner/work/deepmd-kit/deepmd-kit/dp_test/lib/libdeepmd_op_pt.so(+0x9a38a)[0x7f3e940e838a] - # /home/runner/work/deepmd-kit/deepmd-kit/dp_test/lib/libdeepmd_op_pt.so(_Z9border_opRKN2at6TensorES2_S2_S2_S2_S2_S2_S2_S2_+0x8e)[0x7f3e940dda63] - # /home/runner/work/deepmd-kit/deepmd-kit/dp_test/lib/libdeepmd_op_pt.so(+0xaeac3)[0x7f3e940fcac3] pytest.skip(reason="Known segfault, see comments for details")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- source/lmp/tests/run_mpi_pair_deepmd.py (2 hunks)
- source/lmp/tests/test_lammps_dpa_pt_nopbc.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
source/lmp/tests/run_mpi_pair_deepmd.py (1)
24-24: LGTM: New command-line argument added correctly.The addition of the
--nopbcargument as a boolean flag is implemented correctly and aligns with the PR objectives. The argument name is clear and descriptive.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@ ## devel #4220 +/- ## ======================================= Coverage 83.50% 83.50% ======================================= Files 541 541 Lines 52486 52486 Branches 3043 3047 +4 ======================================= + Hits 43830 43831 +1 Misses 7708 7708 + Partials 948 947 -1 ☔ View full report in Codecov by Sentry. |
Adding tests to see whether #4167 is resolved. The answer is no. Segfaults are thrown with MPI. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new command-line argument `--nopbc` to modify boundary conditions in LAMMPS simulations. - **Tests** - Added a comprehensive suite of unit tests for the DeepMD potential in LAMMPS, covering various configurations and scenarios to ensure accuracy and reliability. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
github-merge-queue bot removed this pull request from the merge queue due to failed status checks 
…4237) fix errors mentioned in following pr: #4220 #4209 #4144 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced message passing logic in the computation process for improved efficiency. - Added new test functions to evaluate DeepMD model performance under various conditions. - **Bug Fixes** - Improved error handling and assertions in test cases to ensure robustness. - **Refactor** - Streamlined tensor operations in the communication process to enhance clarity and reduce unnecessary computations. - Removed outdated test cases related to neighbor list handling in the DeepPot class. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
4 participants
Adding tests to see whether #4167 is resolved. The answer is no. Segfaults are thrown with MPI.
Summary by CodeRabbit
--nopbcto modify boundary conditions in LAMMPS simulations.