feat(c++,pt-expt): add DeepPotModelDevi C++ tests for .pt2 backend#5342
Open
wanghan-iapcm wants to merge 2 commits intodeepmodeling:masterfrom
Open
feat(c++,pt-expt): add DeepPotModelDevi C++ tests for .pt2 backend#5342wanghan-iapcm wants to merge 2 commits intodeepmodeling:masterfrom
wanghan-iapcm wants to merge 2 commits intodeepmodeling:masterfrom
Conversation
Add model deviation tests using two SE(A)+fparam/aparam .pt2 models with different seeds. Tests cover attrs, build_nlist, lmp_list, atomic variants, and std/avg/relative_std deviation statistics with precomputed references.
Contributor
📝 WalkthroughWalkthroughAdds end-to-end test support for DeepPotModelDevi with the PyTorch Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/api_cc/tests/test_deeppot_model_devi_ptexpt.cc`:
- Around line 56-72: The test asserts dp_md.has_default_fparam() but never
exercises the default-parameter path; add a short subcase inside the
TestInferDeepPotModeDeviPtExpt(attrs) test that calls the
DeepPotModelDevi::compute(...) overload that omits fparam (i.e., invoke
dp_md.compute(...) with the same inputs but without passing fparam) and assert
the expected behavior (e.g., no throw and outputs match the explicit-fparam call
or whatever the regression expects). Locate dp_md and the existing compute calls
in this test and add the empty-fparam invocation and corresponding EXPECT_*
checks to lock down the real backend behavior.
- Around line 6-17: The file defines EPSILON using std::is_same (and VALUETYPE)
but does not include <type_traits>, relying on transitive includes; add a direct
`#include` <type_traits> near the other includes so std::is_same resolves reliably
for the EPSILON macro definition (refer to the EPSILON macro and the use of
std::is_same/VALUETYPE).
In `@source/tests/infer/gen_model_devi.py`:
- Around line 137-144: The test unpacks multiple values from dp.eval and later
in the generator many of those bindings are unused (v, e0, ae0, av0, e1, ae1,
av1) and the loop index is unused, causing Ruff errors; rename any unused
variables returned by dp.eval (e.g., v -> _v, e0 -> _e0, ae0 -> _ae0, av0 ->
_av0, e1 -> _e1, ae1 -> _ae1, av1 -> _av1) or remove/underscore the unpacked
names you don't use, and if an enumerate loop index is unused replace
enumerate(...) with iterating values only (or rename the index to _i) so the
generator in gen_model_devi.py (the dp.eval unpack and the loop around line
~200) passes ruff check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 17471cf5-6361-47c2-8791-1ea8d8b66149
📒 Files selected for processing (3)
source/api_cc/tests/test_deeppot_model_devi_ptexpt.ccsource/install/test_cc_local.shsource/tests/infer/gen_model_devi.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5342 +/- ##
==========================================
- Coverage 82.28% 82.27% -0.02%
==========================================
Files 797 798 +1
Lines 82100 82450 +350
Branches 4003 4050 +47
==========================================
+ Hits 67557 67835 +278
- Misses 13336 13406 +70
- Partials 1207 1209 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
All other gen scripts import from deepmd.pt.utils.serialization before calling load_custom_ops(), which loads the installed libdeepmd_op_pt.so first. The guard in load_custom_ops() then skips the build-dir copy. gen_model_devi.py was missing this import, so load_custom_ops() loaded the build-dir .so first, then the later deepmd.infer import loaded the installed .so — triggering a duplicate op registration crash.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
source/tests/infer/gen_model_devi.py (1)
140-140:⚠️ Potential issue | 🟠 MajorFix unresolved Ruff violations for unused unpacked values and loop index.
v,e0,ae0,av0,e1,ae1,av1, andkkare unused, which triggersRUF059/B007and can fail CI.♻️ Proposed fix
- e, f, v, ae, av = dp.eval( + e, f, _v, ae, av = dp.eval( @@ - e0, f0, v0, ae0, av0 = dp0.eval( + _e0, f0, v0, _ae0, _av0 = dp0.eval( coord, box, atype, fparam=fparam_val, aparam=aparam_val, atomic=True ) - e1, f1, v1, ae1, av1 = dp1.eval( + _e1, f1, v1, _ae1, _av1 = dp1.eval( coord, box, atype, fparam=fparam_val, aparam=aparam_val, atomic=True ) @@ - for kk, fk in enumerate([f0_flat, f1_flat]): + for fk in (f0_flat, f1_flat): for dd in range(3):As per coding guidelines,
**/*.py: Always runruff check .andruff format .before committing changes or CI will fail.Also applies to: 184-187, 203-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/tests/infer/gen_model_devi.py` at line 140, Unresolved Ruff violations come from unused unpacked variables (v, e0, ae0, av0, e1, ae1, av1) and an unused loop index kk; update the unpacking and loop to use underscore placeholders or underscore-prefixed names to mark them as intentionally unused. For example, change the dp.eval(...) unpack to use underscores for unused slots (e.g., _, f, _, ae, _ = dp.eval(...)) and rename loop index kk to _ or _kk where it isn't used; apply the same underscore pattern to the other unpackings referenced (the blocks around the unpackings that create e0/ae0/av0 and e1/ae1/av1) to satisfy RUF059/B007. Ensure you only change the variable names where they are unused so real uses (like f, ae) remain intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@source/tests/infer/gen_model_devi.py`:
- Line 140: Unresolved Ruff violations come from unused unpacked variables (v,
e0, ae0, av0, e1, ae1, av1) and an unused loop index kk; update the unpacking
and loop to use underscore placeholders or underscore-prefixed names to mark
them as intentionally unused. For example, change the dp.eval(...) unpack to use
underscores for unused slots (e.g., _, f, _, ae, _ = dp.eval(...)) and rename
loop index kk to _ or _kk where it isn't used; apply the same underscore pattern
to the other unpackings referenced (the blocks around the unpackings that create
e0/ae0/av0 and e1/ae1/av1) to satisfy RUF059/B007. Ensure you only change the
variable names where they are unused so real uses (like f, ae) remain intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f10a1f7b-5d59-4bb2-91ab-8684d6404121
📒 Files selected for processing (1)
source/tests/infer/gen_model_devi.py
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
1 participant
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
DeepPotModelDeviunit tests for the pt_expt (.pt2) backendgen_model_devi.py) for meaningful deviationsdefault_fparam=[0.25852028]for attribute detection testsTests added (14 total: 2 test classes × {double, float})
TestInferDeepPotModeDeviPtExpt (6 tests per type):
attrs— cutoff, numb_types, dim_fparam==1, dim_aparam==1, has_default_fparamcpu_build_nlist/cpu_build_nlist_atomic— individual dp0/dp1 vs model_devi consistencycpu_lmp_list/cpu_lmp_list_atomic— same with LAMMPS nlist + fold_backcpu_lmp_list_std— manual vs API compute_avg, compute_std_e, compute_std_f, compute_relative_std_fTestInferDeepPotModeDeviPtExptPrecomputed (1 test per type):
cpu_lmp_list_std— max/min/avg std_f and max/min/mystd std_v against hardcoded reference valuesKnown limitations
default_fparamcompute path is not tested:DeepPotPTExpt::computecreates a zero-size tensor when fparam is empty, which crashes the .pt2 runner. The C++ API supportshas_default_fparam()detection but not automatic value substitution.Test plan
gen_model_devi.pygenerates two .pt2 models without errorsSummary by CodeRabbit