test(consistent): preserve legacy parameterized semantics#5388
Closed
njzjz-bot wants to merge 1 commit intodeepmodeling:masterfrom
Closed
test(consistent): preserve legacy parameterized semantics#5388njzjz-bot wants to merge 1 commit intodeepmodeling:masterfrom
njzjz-bot wants to merge 1 commit intodeepmodeling:masterfrom
Conversation
Adopt the curated descriptor matrices from PR deepmodeling#5378, keep the new parameterized_cases helper, and preserve the legacy parameterized() class-name behavior for existing tests. This follows option A from review discussion: add an explicit helper for curated cases without changing the original Cartesian-product helper semantics. Authored by OpenClaw (model: gpt-5.4)
Contributor
📝 WalkthroughWalkthroughThis pull request introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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: 2
🧹 Nitpick comments (1)
source/tests/consistent/common.py (1)
779-797: Consider adding a shared validated case-builder next toparameterized_cases().The new decorator is shared, but DPA1/DPA2/SeA each still carry their own
*_CASE_FIELDS/ baseline /*_case()trio. A small helper here that materializes tuples from a baseline and rejects unknown override keys would remove that duplication and turn typoed overrides into immediate failures instead of silently shrinking curated coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/tests/consistent/common.py` around lines 779 - 797, Add a shared, validated case-builder to centralize the pattern used by DPA1/DPA2/SeA: implement a helper (e.g., build_validated_cases or make_case_tuples) adjacent to parameterized_cases that takes a baseline mapping, a list of override dicts, and an explicit allowed/ordered field list (the current *_CASE_FIELDS), validates that each override contains only known keys (raise on unknown keys), merges overrides into the baseline in the specified field order and returns tuples suitable for _parameterized_with_cases; update DPA1/DPA2/SeA to call this helper instead of duplicating their baseline/ *_case() trio so typos in override keys fail fast and tuple ordering remains consistent.
🤖 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/tests/consistent/descriptor/test_dpa1.py`:
- Around line 83-126: The curated cases all inherit smooth_type_embedding=True
from DPA1_BASELINE_CASE which causes skip_tf() to exclude TensorFlow tests;
update DPA1_CURATED_CASES to include at least one TF-compatible entry by calling
dpa1_case(...) with smooth_type_embedding=False (e.g. add
dpa1_case(smooth_type_embedding=False) or another existing case variant that
specifies smooth_type_embedding=False) so TestDPA1.test_tf_* can run.
In `@source/tests/consistent/descriptor/test_dpa2.py`:
- Line 275: The tuple destructures in test_dpa2.py include an unused binding
repformer_update_g1_has_conv which triggers linter errors; update each
destructure (including the occurrences around the listed sites) to either remove
that element from the unpack pattern if not needed or rename it to
_repformer_update_g1_has_conv to mark it intentionally ignored (search for
repformer_update_g1_has_conv in the file and adjust the unpack in the helper
calls such as the tuples used by the methods that only use precision or nothing
from self.param), then run ruff check . to ensure no remaining unused-binding
warnings.
---
Nitpick comments:
In `@source/tests/consistent/common.py`:
- Around line 779-797: Add a shared, validated case-builder to centralize the
pattern used by DPA1/DPA2/SeA: implement a helper (e.g., build_validated_cases
or make_case_tuples) adjacent to parameterized_cases that takes a baseline
mapping, a list of override dicts, and an explicit allowed/ordered field list
(the current *_CASE_FIELDS), validates that each override contains only known
keys (raise on unknown keys), merges overrides into the baseline in the
specified field order and returns tuples suitable for _parameterized_with_cases;
update DPA1/DPA2/SeA to call this helper instead of duplicating their baseline/
*_case() trio so typos in override keys fail fast and tuple ordering remains
consistent.
🪄 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: 0b67d4e7-e263-4e27-b5f4-17b2c62ceeb9
📒 Files selected for processing (4)
source/tests/consistent/common.pysource/tests/consistent/descriptor/test_dpa1.pysource/tests/consistent/descriptor/test_dpa2.pysource/tests/consistent/descriptor/test_se_e2_a.py
Comment on lines
+83
to
126
| DPA1_BASELINE_CASE = { | ||
| "tebd_dim": 4, | ||
| "tebd_input_mode": "concat", | ||
| "resnet_dt": True, | ||
| "type_one_side": True, | ||
| "attn": 20, | ||
| "attn_layer": 2, | ||
| "attn_dotr": True, | ||
| "excluded_types": [], | ||
| "env_protection": 0.0, | ||
| "set_davg_zero": True, | ||
| "scaling_factor": 1.0, | ||
| "normalize": True, | ||
| "temperature": 1.0, | ||
| "ln_eps": 1e-5, | ||
| "smooth_type_embedding": True, | ||
| "concat_output_tebd": True, | ||
| "precision": "float64", | ||
| "use_econf_tebd": False, | ||
| "use_tebd_bias": False, | ||
| } | ||
|
|
||
|
|
||
| def dpa1_case(**overrides: Any) -> tuple: | ||
| case = DPA1_BASELINE_CASE | overrides | ||
| return tuple(case[field] for field in DPA1_CASE_FIELDS) | ||
|
|
||
|
|
||
| DPA1_CURATED_CASES = ( | ||
| # Baseline coverage. | ||
| dpa1_case(), | ||
| # Alternate tebd input plumbing. | ||
| dpa1_case(tebd_input_mode="strip"), | ||
| # High-risk descriptor toggles. | ||
| dpa1_case(excluded_types=[[0, 1]]), | ||
| dpa1_case(set_davg_zero=False), | ||
| dpa1_case(normalize=False), | ||
| # Attention edge cases: disabled temperature path vs zero-layer path. | ||
| dpa1_case(temperature=None), | ||
| dpa1_case(attn_layer=0, temperature=None), | ||
| # econf-specific path with both tebd input modes. | ||
| dpa1_case(use_econf_tebd=True), | ||
| dpa1_case(tebd_input_mode="strip", use_econf_tebd=True), | ||
| ) |
Contributor
There was a problem hiding this comment.
The curated DPA1 matrix now skips TensorFlow entirely.
skip_tf() treats smooth_type_embedding=True as unsupported, and every entry in DPA1_CURATED_CASES inherits that value from the baseline. So TestDPA1.test_tf_* will never execute anymore. Add at least one TF-compatible curated case if this suite is still supposed to cover TensorFlow.
💡 Minimal fix
DPA1_CURATED_CASES = (
# Baseline coverage.
dpa1_case(),
+ # Keep one TF-compatible case alive.
+ dpa1_case(smooth_type_embedding=False),
# Alternate tebd input plumbing.
dpa1_case(tebd_input_mode="strip"),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| DPA1_BASELINE_CASE = { | |
| "tebd_dim": 4, | |
| "tebd_input_mode": "concat", | |
| "resnet_dt": True, | |
| "type_one_side": True, | |
| "attn": 20, | |
| "attn_layer": 2, | |
| "attn_dotr": True, | |
| "excluded_types": [], | |
| "env_protection": 0.0, | |
| "set_davg_zero": True, | |
| "scaling_factor": 1.0, | |
| "normalize": True, | |
| "temperature": 1.0, | |
| "ln_eps": 1e-5, | |
| "smooth_type_embedding": True, | |
| "concat_output_tebd": True, | |
| "precision": "float64", | |
| "use_econf_tebd": False, | |
| "use_tebd_bias": False, | |
| } | |
| def dpa1_case(**overrides: Any) -> tuple: | |
| case = DPA1_BASELINE_CASE | overrides | |
| return tuple(case[field] for field in DPA1_CASE_FIELDS) | |
| DPA1_CURATED_CASES = ( | |
| # Baseline coverage. | |
| dpa1_case(), | |
| # Alternate tebd input plumbing. | |
| dpa1_case(tebd_input_mode="strip"), | |
| # High-risk descriptor toggles. | |
| dpa1_case(excluded_types=[[0, 1]]), | |
| dpa1_case(set_davg_zero=False), | |
| dpa1_case(normalize=False), | |
| # Attention edge cases: disabled temperature path vs zero-layer path. | |
| dpa1_case(temperature=None), | |
| dpa1_case(attn_layer=0, temperature=None), | |
| # econf-specific path with both tebd input modes. | |
| dpa1_case(use_econf_tebd=True), | |
| dpa1_case(tebd_input_mode="strip", use_econf_tebd=True), | |
| ) | |
| DPA1_BASELINE_CASE = { | |
| "tebd_dim": 4, | |
| "tebd_input_mode": "concat", | |
| "resnet_dt": True, | |
| "type_one_side": True, | |
| "attn": 20, | |
| "attn_layer": 2, | |
| "attn_dotr": True, | |
| "excluded_types": [], | |
| "env_protection": 0.0, | |
| "set_davg_zero": True, | |
| "scaling_factor": 1.0, | |
| "normalize": True, | |
| "temperature": 1.0, | |
| "ln_eps": 1e-5, | |
| "smooth_type_embedding": True, | |
| "concat_output_tebd": True, | |
| "precision": "float64", | |
| "use_econf_tebd": False, | |
| "use_tebd_bias": False, | |
| } | |
| def dpa1_case(**overrides: Any) -> tuple: | |
| case = DPA1_BASELINE_CASE | overrides | |
| return tuple(case[field] for field in DPA1_CASE_FIELDS) | |
| DPA1_CURATED_CASES = ( | |
| # Baseline coverage. | |
| dpa1_case(), | |
| # Keep one TF-compatible case alive. | |
| dpa1_case(smooth_type_embedding=False), | |
| # Alternate tebd input plumbing. | |
| dpa1_case(tebd_input_mode="strip"), | |
| # High-risk descriptor toggles. | |
| dpa1_case(excluded_types=[[0, 1]]), | |
| dpa1_case(set_davg_zero=False), | |
| dpa1_case(normalize=False), | |
| # Attention edge cases: disabled temperature path vs zero-layer path. | |
| dpa1_case(temperature=None), | |
| dpa1_case(attn_layer=0, temperature=None), | |
| # econf-specific path with both tebd input modes. | |
| dpa1_case(use_econf_tebd=True), | |
| dpa1_case(tebd_input_mode="strip", use_econf_tebd=True), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/tests/consistent/descriptor/test_dpa1.py` around lines 83 - 126, The
curated cases all inherit smooth_type_embedding=True from DPA1_BASELINE_CASE
which causes skip_tf() to exclude TensorFlow tests; update DPA1_CURATED_CASES to
include at least one TF-compatible entry by calling dpa1_case(...) with
smooth_type_embedding=False (e.g. add dpa1_case(smooth_type_embedding=False) or
another existing case variant that specifies smooth_type_embedding=False) so
TestDPA1.test_tf_* can run.
| repinit_use_three_body, | ||
| repformer_update_g1_has_conv, | ||
| repformer_direct_dist, | ||
| repformer_update_g1_has_conv, |
Contributor
There was a problem hiding this comment.
Drop or underscore the new dead bindings in these tuple destructures.
repformer_update_g1_has_conv is not read at any of these sites, and most of these helpers only use precision or nothing from self.param. Static analysis is already flagging each occurrence, so either unpack only the fields each method needs or rename this slot to _repformer_update_g1_has_conv where it is intentionally ignored. As per coding guidelines, **/*.py: Install linter and run ruff check . before committing changes or the CI will fail.
Also applies to: 308-308, 341-341, 374-374, 451-451, 557-557, 596-596
🧰 Tools
🪛 Ruff (0.15.9)
[warning] 275-275: Unpacked variable repformer_update_g1_has_conv is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/tests/consistent/descriptor/test_dpa2.py` at line 275, The tuple
destructures in test_dpa2.py include an unused binding
repformer_update_g1_has_conv which triggers linter errors; update each
destructure (including the occurrences around the listed sites) to either remove
that element from the unpack pattern if not needed or rename it to
_repformer_update_g1_has_conv to mark it intentionally ignored (search for
repformer_update_g1_has_conv in the file and adjust the unpack in the helper
calls such as the tuples used by the methods that only use precision or nothing
from self.param), then run ruff check . to ensure no remaining unused-binding
warnings.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5388 +/- ##
=======================================
Coverage 80.45% 80.45%
=======================================
Files 814 814
Lines 84438 84438
Branches 4050 4051 +1
=======================================
+ Hits 67935 67936 +1
Misses 15279 15279
+ Partials 1224 1223 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
parameterized_cases()for explicit curated case lists, including sanitized/collision-safe generated test namesparameterized()Cartesian-product naming semantics unchangedWhy
In the review discussion for #5378, option A was to keep the original
parameterized()behavior stable and introduce a separate helper for curated-case suites. This PR implements that split directly:parameterized()keep the old behaviorparameterized_cases()That keeps the CI-matrix reduction and reviewable curated coverage from #5378 without changing legacy semantics for unrelated tests.
Notes
type_one_side=Falsecase added later in reviewAuthored by OpenClaw (model: gpt-5.4)
Summary by CodeRabbit
parameterized_casesdecorator for flexible test parameterization.