test(universal): replace Cartesian product with curated matrices for descriptor test#5459
test(universal): replace Cartesian product with curated matrices for descriptor test#5459iProzd wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR reduces the combinatorial explosion in universal model/atomic-model descriptor integration tests by replacing broad Cartesian-product parameter grids with curated descriptor variant matrices aimed at representative/high-risk coverage.
Changes:
- Introduces curated “EnergyModel” descriptor variant lists (plus a small helper to generate variants) in the universal dpmodel descriptor test module.
- Refactors universal PT/DP model and atomic-model tests to consume these curated descriptor matrices and shared default descriptor sets instead of importing the full parameter grids.
- Adds per-test-module constants grouping descriptor/fitting parameterizations (e.g., default vs. hybrid-inclusive sets) to simplify parameterized test definitions.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| source/tests/universal/pt/model/test_model.py | Switches model tests to curated descriptor matrices and introduces shared descriptor parameter group constants. |
| source/tests/universal/pt/atomc_model/test_atomic_model.py | Switches atomic-model tests to curated descriptor matrices and shared descriptor parameter group constants. |
| source/tests/universal/dpmodel/model/test_model.py | Switches dpmodel model tests to curated descriptor matrices and adds shared descriptor parameter group constants. |
| source/tests/universal/dpmodel/descriptor/test_descriptor.py | Adds helper + curated descriptor variant lists for model/atomic-model integration tests. |
| source/tests/universal/dpmodel/atomc_model/test_atomic_model.py | Switches dpmodel atomic-model tests to curated descriptor matrices and shared descriptor parameter group constants. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
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 consolidates descriptor parameterization logic across test modules by introducing energy-model-specific descriptor parameter lists and shared module-level constants. Tests import curated parameter variants and use precomputed descriptor-parameter tuples instead of inline expansions, reducing repetition across DP and PT test classes. ChangesTest parameterization refactoring for descriptor model tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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.
🧹 Nitpick comments (1)
source/tests/universal/pt/model/test_model.py (1)
631-636: ⚡ Quick winAvoid positional slicing for spin fit descriptor selection.
DEFAULT_SPIN_DESCRIPTOR_PARAMS[:6]is order-coupled and can silently change test coverage if the tuple is reordered. Define an explicit spin-fit constant and reference it directly.Suggested refactor
DEFAULT_SPIN_DESCRIPTOR_PARAMS = ( (DescriptorParamSeA, DescrptSeA), (DescriptorParamSeR, DescrptSeR), (DescriptorParamSeT, DescrptSeT), (DescriptorParamSeTTebd, DescrptSeTTebd), (DescriptorParamDPA1, DescrptDPA1), (DescriptorParamDPA2, DescrptDPA2), # unsupported for SpinModel to hybrid both mixed_types and no-mixed_types descriptor (DescriptorParamHybridMixed, DescrptHybrid), (DescriptorParamHybridMixedTTebd, DescrptHybrid), ) + +DEFAULT_SPIN_FIT_DESCRIPTOR_PARAMS = ( + (DescriptorParamSeA, DescrptSeA), + (DescriptorParamSeR, DescrptSeR), + (DescriptorParamSeT, DescrptSeT), + (DescriptorParamSeTTebd, DescrptSeTTebd), + (DescriptorParamDPA1, DescrptDPA1), + (DescriptorParamDPA2, DescrptDPA2), +) @@ fit_parameterized=( - DEFAULT_SPIN_DESCRIPTOR_PARAMS[:6], # descrpt_class_param & class + DEFAULT_SPIN_FIT_DESCRIPTOR_PARAMS, # descrpt_class_param & class ( *[(param_func, EnergyFittingNet) for param_func in FittingParamEnergyList], ), # fitting_class_param & class ), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/tests/universal/pt/model/test_model.py` around lines 631 - 636, Replace the positional slice DEFAULT_SPIN_DESCRIPTOR_PARAMS[:6] with an explicit constant to avoid order coupling: introduce a named tuple/constant like SPIN_FIT_DESCRIPTOR_PARAMS (or DEFAULT_SPIN_DESCRIPTOR_PARAMS_SPIN_FIT) defined next to DEFAULT_SPIN_DESCRIPTOR_PARAMS and use that constant in the fit_parameterized case instead of slicing; update any references in the test (e.g., fit_parameterized) to use the new constant so test coverage remains stable even if DEFAULT_SPIN_DESCRIPTOR_PARAMS is reordered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@source/tests/universal/pt/model/test_model.py`:
- Around line 631-636: Replace the positional slice
DEFAULT_SPIN_DESCRIPTOR_PARAMS[:6] with an explicit constant to avoid order
coupling: introduce a named tuple/constant like SPIN_FIT_DESCRIPTOR_PARAMS (or
DEFAULT_SPIN_DESCRIPTOR_PARAMS_SPIN_FIT) defined next to
DEFAULT_SPIN_DESCRIPTOR_PARAMS and use that constant in the fit_parameterized
case instead of slicing; update any references in the test (e.g.,
fit_parameterized) to use the new constant so test coverage remains stable even
if DEFAULT_SPIN_DESCRIPTOR_PARAMS is reordered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4b441538-8372-4b9d-b23b-981980feb384
📒 Files selected for processing (5)
source/tests/universal/dpmodel/atomc_model/test_atomic_model.pysource/tests/universal/dpmodel/descriptor/test_descriptor.pysource/tests/universal/dpmodel/model/test_model.pysource/tests/universal/pt/atomc_model/test_atomic_model.pysource/tests/universal/pt/model/test_model.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5459 +/- ##
=======================================
Coverage 82.46% 82.46%
=======================================
Files 829 829
Lines 88763 88762 -1
Branches 4225 4227 +2
=======================================
Hits 73197 73197
+ Misses 14274 14273 -1
Partials 1292 1292 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I reviewed this PR. It only trims and deduplicates the test parameter matrices, and the overall direction looks sound. I did not find any issue that should block merging.
Checked:
- Diff across the 5 changed files (+541/-377)
- The kwargs override order for descriptor curated variants: fixed kwargs now override the kwargs passed by the test framework, which matches the intended semantics for fixed variants
- The DPA3
chg/spinspecial cases have been folded into the curated lists, with no remaining stale references - The ordering-coupled
DEFAULT_SPIN_DESCRIPTOR_PARAMS[:6]usage was replaced with an explicit constant as suggested by CodeRabbit - CI, Codecov, pre-commit, and docs are green; I also ran
python3 -m py_compilelocally on the changed Python files successfully
Conclusion: approved / ready to merge.
— OpenClaw 2026.5.12 (model: custom-chat-jinzhezeng-group/gpt-5.5)
| DescriptorParamDPA1, "DescriptorParamDPA1_strip", tebd_input_mode="strip" | ||
| ), | ||
| _descriptor_param_variant( | ||
| DescriptorParamDPA1, "DescriptorParamDPA1_no_attention", attn_layer=0 |
There was a problem hiding this comment.
Coverage gap: _no_attention duplicates the rebound baseline. At line 322 above, DescriptorParamDPA1 is rebound to DescriptorParamDPA1List[0], which has attn_layer=0 (the first value in the OrderedDict tuple (0, 2)). This variant then explicitly sets attn_layer=0 again, and no other variant in DescriptorParamDPA1EnergyModelList overrides it.
Net effect: after this PR, the DPA1 attention code path (attn_layer=2, NeighborGatedAttention) is no longer exercised at the model / atomic-model integration level — only at the descriptor level. Descriptor-internal regressions are still caught by the preserved Cartesian DescriptorParamDPA1List, but model-level glue bugs that surface only with attention enabled would slip through.
Non-blocking; trivial fix: change this variant to attn_layer=2 (and rename to _with_attention) so the curated list covers both branches at the model level.
Summary by CodeRabbit