fix(dpa): handle NULL type properly in lammps plugin#5268
fix(dpa): handle NULL type properly in lammps plugin#5268link89 wants to merge 9 commits intodeepmodeling:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMap "NULL" atom types to sentinel -1 instead of type_map.size() in DeepMD-related pair/fix code; add unit tests exercising Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
source/lmp/tests/test_lammps_dpa_pt.py (1)
497-500: Consider adding a basic sanity assertion.The test is a crash-regression smoke test (no assertions), which is acceptable given the bug being fixed (double-free). However, since
run(0)with only H atoms active is a well-defined state, adding even a simpleassert lammps_type_map.eval("pe") != 0(or apytest.approxagainst a known value) would make the test more meaningful and protect against silent incorrect behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lmp/tests/test_lammps_dpa_pt.py` around lines 497 - 500, In test_pair_deepmd_type_map_with_null, after calling lammps_type_map.run(0) add a simple sanity assertion that checks the computed potential energy is sensible (for example assert lammps_type_map.eval("pe") != 0 or use pytest.approx against a known value); place the assertion immediately after lammps_type_map.run(0) to ensure the test still guards against silent incorrect behavior while keeping it as a smoke/regression test for the double-free fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/lmp/tests/test_lammps_dpa_pt.py`:
- Around line 495-502: Add the missing blank line separating top-level test
functions: ensure there are two blank lines between the function definitions
test_pair_deepmd_type_map_with_null and test_pair_deepmd_real by inserting one
more blank line after the end of test_pair_deepmd_type_map_with_null (the
lammps_type_map.run(0) line), then run ruff format/check to confirm PEP8 E302 is
satisfied.
---
Nitpick comments:
In `@source/lmp/tests/test_lammps_dpa_pt.py`:
- Around line 497-500: In test_pair_deepmd_type_map_with_null, after calling
lammps_type_map.run(0) add a simple sanity assertion that checks the computed
potential energy is sensible (for example assert lammps_type_map.eval("pe") != 0
or use pytest.approx against a known value); place the assertion immediately
after lammps_type_map.run(0) to ensure the test still guards against silent
incorrect behavior while keeping it as a smoke/regression test for the
double-free fix.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
source/lmp/fix_dplr.cppsource/lmp/pair_deepmd.cppsource/lmp/pair_deepspin.cppsource/lmp/tests/test_lammps_dpa_pt.py
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Pull request overview
Fixes handling of NULL atom types in DeepMD-related LAMMPS plugins by using a sentinel mapping value, and adds a regression test to cover the NULL type-map case.
Changes:
- Map
NULLtypes to-1instead oftype_map.size()in multiple LAMMPS plugin components. - Add a LAMMPS Python test to ensure
pair_coeffaccepts aNULLtype entry without crashing.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| source/lmp/tests/test_lammps_dpa_pt.py | Adds a regression test for NULL in the DeepMD type map. |
| source/lmp/pair_deepspin.cpp | Changes NULL type index mapping to -1 sentinel. |
| source/lmp/pair_deepmd.cpp | Changes NULL type index mapping to -1 sentinel. |
| source/lmp/fix_dplr.cpp | Changes NULL type index mapping to -1 sentinel. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: link89 <xuweihong.cn@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/lmp/tests/test_lammps_dpa_pt.py`:
- Around line 498-501: The current test test_pair_deepmd_type_map_with_null only
uses pair_style("deepmd ...") and doesn't exercise the hybrid/scaled + NULL
combination reported in Issue `#4749`; update or add a test that sets pair_style
to a hybrid/scaled form that includes deepmd (e.g., pair_style("hybrid/scaled
deepmd ...")) and then call pair_coeff with the same NULL mapping
(lammps_type_map.pair_coeff("* * H O NULL")) and run(0) to ensure the
hybrid/scaled + NULL path is exercised and does not crash; locate the test
function test_pair_deepmd_type_map_with_null and modify its pair_style
invocation (or create a new test with a distinct name) to use the hybrid/scaled
variant so the exact failing configuration is covered.
Signed-off-by: link89 <xuweihong.cn@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
source/lmp/tests/test_lammps.py (1)
520-524: Smoke test has no assertions — consider verifying forces on non-NULL atoms or zero forces on NULL atoms.The test correctly exercises the NULL type code path (type-2 atoms in
data_type_map_fileare mapped to NULL), making it an effective crash-regression guard for issue#4749. However, without any assertion, a silent wrong-result regression (e.g., non-zero forces being computed for NULL atoms) would go undetected.A lightweight addition would be to assert that atoms of the NULL type receive zero force:
💡 Suggested assertion for NULL-type atoms
def test_pair_deepmd_type_map_with_null(lammps_type_map) -> None: lammps_type_map.pair_style(f"deepmd {pb_file.resolve()}") lammps_type_map.pair_coeff("* * H NULL") lammps_type_map.run(0) + # type_HO maps type 2 → NULL; atoms at 0-based indices 0 and 3 are type 2 + null_atom_ids = [ + i for i, t in enumerate(type_HO) if t == 2 + ] # 0-based positions of NULL atoms + for ii in range(len(lammps_type_map.atoms)): + atom = lammps_type_map.atoms[ii] + if (atom.id - 1) in null_atom_ids: + assert atom.force == pytest.approx([0.0, 0.0, 0.0])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lmp/tests/test_lammps.py` around lines 520 - 524, The test function test_pair_deepmd_type_map_with_null currently only runs LAMMPS without assertions; update it to fetch the computed forces after lammps_type_map.run(0) and assert that atoms mapped to NULL (type-2 in the data_type_map_file) have zero force (or that non-NULL atoms have non-zero forces) to prevent silent regressions — locate the test by function name and add a small check using the LAMMPS wrapper's force-accessor (or API used elsewhere in tests) after calling lammps_type_map.run(0) to verify per-atom forces for NULL vs non-NULL types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@source/lmp/tests/test_lammps.py`:
- Around line 520-524: The test function test_pair_deepmd_type_map_with_null
currently only runs LAMMPS without assertions; update it to fetch the computed
forces after lammps_type_map.run(0) and assert that atoms mapped to NULL (type-2
in the data_type_map_file) have zero force (or that non-NULL atoms have non-zero
forces) to prevent silent regressions — locate the test by function name and add
a small check using the LAMMPS wrapper's force-accessor (or API used elsewhere
in tests) after calling lammps_type_map.run(0) to verify per-atom forces for
NULL vs non-NULL types.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5268 +/- ##
==========================================
- Coverage 82.00% 80.06% -1.94%
==========================================
Files 750 750
Lines 75082 75173 +91
Branches 3615 3611 -4
==========================================
- Hits 61571 60190 -1381
- Misses 12347 13996 +1649
+ Partials 1164 987 -177 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: link89 <xuweihong.cn@gmail.com>
Signed-off-by: link89 <xuweihong.cn@gmail.com>
Signed-off-by: link89 <xuweihong.cn@gmail.com>
fix: #4749
related to: #4889
Summary by CodeRabbit
Bug Fixes
Tests