test(infer,lmp): drop stale DPA2 .pth fixtures, repoint tests to deeppot_dpa2#5446
Open
wanghan-iapcm wants to merge 4 commits into
Open
test(infer,lmp): drop stale DPA2 .pth fixtures, repoint tests to deeppot_dpa2#5446wanghan-iapcm wants to merge 4 commits into
wanghan-iapcm wants to merge 4 commits into
Conversation
added 2 commits
May 18, 2026 20:02
…eeppot_dpa2 The pre-committed deeppot_dpa.pth and deeppot_dpa_sel.pth predate the g1_out_mlp / g1_out_conv repformer features, so they cannot be loaded via get_model() + load_state_dict() and therefore can't be converted to .pt2. LAMMPS .pth tests still passed only because torch.jit.load restores the baked-in TorchScript IR. Drop both stale fixtures and point the three LAMMPS .pth tests (test_lammps_dpa_pt.py, _nopbc.py, _sel_pt.py) at the current-arch deeppot_dpa2.pth, loading reference values from deeppot_dpa2.expected via read_expected_ref instead of ~150 hard-coded floats per file. Also delete test_deeppot_dpa_pt.cc — fully redundant with test_deeppot_dpa2_pt.cc, which already exercises the C++ .pth backend on the fresh fixture via expected_ref.h. The small-sel-specific semantics of the deleted deeppot_dpa_sel.pth are not preserved; test_lammps_dpa_sel_pt.py now exercises the same model as the PBC variant. .pt2 LAMMPS coverage for DPA2 is already provided by test_lammps_dpa2_pt2.py, so the original blocker for "LAMMPS .pt2 DPA tests" is closed by this cleanup.
…2_*, drop sel duplicate These three tests target the DPA2 descriptor (not generic DPA); align with the existing test_lammps_dpa2_pt2.py / test_lammps_dpa3_* naming so the descriptor is visible from the filename. test_lammps_dpa_sel_pt.py becomes pure duplicate of the renamed test_lammps_dpa2_pt.py once both point at deeppot_dpa2.pth — drop it rather than carry a misleading "sel" name with no small-sel-specific fixture behind it.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR externalizes DPA2 LAMMPS test expectations to ChangesTest Reference Data Externalization
C++ DeepPot DPA2 atomic-output tests
Test File Removals
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e9bb240a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
added 2 commits
May 18, 2026 22:31
…ot_dpa_pt.cc
The previous commit deleted test_deeppot_dpa_pt.cc on the basis that
test_deeppot_dpa2_pt.cc already covered the same backend / fixture.
That's true for the PBC suite, but the NoPbc suite in the kept file
only had cpu_build_nlist and cpu_lmp_nlist — the atomic variants
(cpu_build_nlist_atomic, cpu_lmp_nlist_atomic) that the deleted file
exercised for NoPbc were silently lost, leaving per-atom energy and
virial behavior on the no-PBC .pth path uncovered.
Add both atomic NoPbc cases back in test_deeppot_dpa2_pt.cc, mirroring
the existing PBC atomic test and the deleted NoPbc atomic bodies.
Local ctest with --gtest_filter=*Dpa2PtNoPbc* now reports 8/8 PASSED
(double and float x build_nlist{,_atomic} and lmp_nlist{,_atomic}).
Flagged by Codex review on PR deepmodeling#5446.
CodeQL flagged the migrated read_expected_ref blocks for an unused module-level expected_ae variable (test_lammps_dpa2_pt.py:47 and test_lammps_dpa2_pt_nopbc.py:43). The pre-migration files exposed expected_ae for symmetry with similar TF/PT tests, but the LAMMPS test bodies only consume expected_e / expected_f / expected_v — nothing inside the test functions references expected_ae directly. Inline the np.sum over the atomic-energy array and remove the expected_ae binding from both the try and the except branches.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5446 +/- ##
========================================
Coverage 82.48% 82.48%
========================================
Files 830 829 -1
Lines 88514 88402 -112
Branches 4232 4216 -16
========================================
- Hits 73010 72919 -91
+ Misses 14218 14195 -23
- Partials 1286 1288 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
iProzd
approved these changes
May 19, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
deeppot_dpa.pthanddeeppot_dpa_sel.pthfixtures and the redundant C++ testtest_deeppot_dpa_pt.cc. Both.pthfiles predate theg1_out_mlp/g1_out_convrepformer features, so they could be loaded only via the baked-in TorchScript IR (torch.jit.load) — they could not be round-tripped throughget_model() + load_state_dict()and therefore could not be converted to.pt2. This was the blocker recorded inmemory/project_stale_dpa2_pth.mdand the master plan's "LAMMPS .pt2 DPA tests" planned item.deeppot_dpa2.pthand load reference values fromdeeppot_dpa2.expectedviaread_expected_ref(already used bytest_lammps_dpa3_pt2.py). This replaces ~150 hard-coded reference floats per file with a 5-linetry / exceptblock, and the .expected file is regenerated at CI time bysource/tests/infer/gen_dpa2.py.test_lammps_dpa_pt.py→test_lammps_dpa2_pt.pyandtest_lammps_dpa_pt_nopbc.py→test_lammps_dpa2_pt_nopbc.pyso the descriptor is visible from the filename (matchingtest_lammps_dpa2_pt2.py/test_lammps_dpa3_*convention). Deletetest_lammps_dpa_sel_pt.pyoutright — once both fixtures collapse ontodeeppot_dpa2.pthit becomes a pure duplicate of the PBC variant. The small-sel-specific semantics of the olddeeppot_dpa_sel.pthare not preserved; if that coverage is wanted back, the right path is a separatedeeppot_dpa2_sel.pthfixture written by an extendedgen_dpa2.py.test_deeppot_dpa_pt.cc: once migrated todeeppot_dpa2.pth+expected_ref.h, this file is byte-for-byte equivalent to the existingtest_deeppot_dpa2_pt.cc(same backend, same fixture, same geometry, same five test cases). Keeping both would be pure duplication.Net diff: 9 files changed, +61 insertions, −1369 deletions.
The JAX path (
source/lmp/tests/test_lammps_dpa_jax.py,source/api_cc/tests/test_deeppot_dpa_jax.cc, and thedeeppot_dpa.yaml→deeppot_dpa.savedmodelconversion inconvert-models.sh) is intentionally left untouched. Thedeeppot_dpa.yamlhas the current architecture (the .pth files diverged from it at some point), so the JAX tests continue to work; renaming them for consistency would be a separate cleanup.Test plan
pytest --collect-onlyon the two renamed Python tests collects 33 tests (PBC 17 + NoPBC 16). NoModuleNotFoundError,read_expected_refresolves the.expectedfile at import time.DeepPot(deeppot_dpa2.pth).eval(...)output matches every value indeeppot_dpa2.expectedfor both PBC and NoPBC sections (atol=1e-12, rtol=0).*Dpa2Pt*:*Dpa2PtNoPbc*frombuild_pt2_test/: 28 / 28 PASSED in 69.4 s. Covers all.pthcases that the deletedtest_deeppot_dpa_pt.ccwould have run (TestInferDeepPotDpa2Pt,TestInferDeepPotDpa2PtNoPbc, double + float), plus the existing.pt2AOTI suites.convert-models.shstill producesdeeppot_dpa.savedmodelcleanly for the JAX tests (the yaml is unchanged, but worth watching the first CI run).Closes the "Stale DPA2 .pth files" limitation and the "LAMMPS .pt2 DPA tests" planned item in the pt_expt master plan.
🤖 Generated with Claude Code
Summary by CodeRabbit