test(universal): replace Cartesian product with curated matrices for descriptor test#5459
Merged
Conversation
Contributor
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.
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR centralizes descriptor parameterization for DP and PT tests by adding energy-model-specific descriptor parameter lists and shared module-level descriptor/fitting tuples, then rewiring multiple ChangesTest parameterization refactoring for descriptor model tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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.25% 82.25%
=======================================
Files 833 833
Lines 89094 89094
Branches 4225 4225
=======================================
Hits 73288 73288
Misses 14515 14515
Partials 1291 1291 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
njzjz
approved these changes
May 27, 2026
njzjz-bot
approved these changes
May 27, 2026
Contributor
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)
Contributor
|
Actionable comments posted: 0 |
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
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 by CodeRabbit