test(consistent): use curated cases throughout#5606
Open
njzjz wants to merge 3 commits into
Open
Conversation
njzjz
requested review from
iProzd and
wanghan-iapcm
and removed request for
wanghan-iapcm
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 (23)
🚧 Files skipped from review as they are similar to previous changes (18)
📝 WalkthroughWalkthroughAcross the consistent test suites, ChangesMigrate consistent tests to
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)source/tests/consistent/fitting/test_dipole.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.13][ERROR]: unable to find a config; path source/tests/consistent/fitting/test_dos.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.15][ERROR]: unable to find a config; path source/tests/consistent/descriptor/test_se_t.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.20][ERROR]: unable to find a config; path
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/test_type_embedding.py (1)
75-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFail fast on unknown override keys.
A misspelled override is silently ignored here: the merged dict keeps the extra key, but tuple construction only reads
TYPE_EMBEDDING_CASE_FIELDS. That can accidentally leave the baseline value in place and reduce case coverage without any signal.Proposed guard
def type_embedding_case(**overrides: Any) -> tuple: + unknown = set(overrides) - set(TYPE_EMBEDDING_CASE_FIELDS) + if unknown: + raise ValueError(f"Unknown type-embedding case fields: {sorted(unknown)}") case = TYPE_EMBEDDING_BASELINE_CASE | overrides return tuple(case[field] for field in TYPE_EMBEDDING_CASE_FIELDS)🤖 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/consistent/test_type_embedding.py` around lines 75 - 77, The type_embedding_case helper silently ignores misspelled override keys when merging TYPE_EMBEDDING_BASELINE_CASE with overrides. Update type_embedding_case to validate overrides against TYPE_EMBEDDING_CASE_FIELDS before building the tuple, and raise an error for any unknown key so bad inputs fail fast instead of falling back to baseline values. Use the existing type_embedding_case function and TYPE_EMBEDDING_CASE_FIELDS/TYPE_EMBEDDING_BASELINE_CASE symbols to keep the check centralized.
🤖 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.
Inline comments:
In `@source/tests/consistent/model/test_dpa1.py`:
- Around line 58-64: The curated DPA1_ENER test cases no longer cover the
smooth-type-embedding branch, so restore that configuration in
DPA1_ENER_CURATED_CASES and keep the related model coverage in test_dpa1.py.
Update the parameterized_cases setup so the suite still exercises the descriptor
mode wired through data(), using the existing DPA1_ENER_CURATED_CASES and
test_dpa1 symbols to place the new case consistently with the current matrix.
In `@source/tests/consistent/model/test_polar.py`:
- Around line 1221-1229: The new curated exclusion cases in the stat test are
reusing the same sampled inputs across dp, pt, and pt_expt, and
compute_or_load_stat mutates natoms in place when atom_exclude_types is
non-empty. Update the test logic in test_polar to apply the same deepcopy
protection already used in test_ener so each backend gets an independent copy of
self.np_sampled and self.pt_sampled before stat computation.
---
Nitpick comments:
In `@source/tests/consistent/test_type_embedding.py`:
- Around line 75-77: The type_embedding_case helper silently ignores misspelled
override keys when merging TYPE_EMBEDDING_BASELINE_CASE with overrides. Update
type_embedding_case to validate overrides against TYPE_EMBEDDING_CASE_FIELDS
before building the tuple, and raise an error for any unknown key so bad inputs
fail fast instead of falling back to baseline values. Use the existing
type_embedding_case function and
TYPE_EMBEDDING_CASE_FIELDS/TYPE_EMBEDDING_BASELINE_CASE symbols to keep the
check centralized.
🪄 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: a06fd2ff-36be-4203-a399-ee618de680a4
📒 Files selected for processing (27)
source/tests/consistent/descriptor/test_se_atten_v2.pysource/tests/consistent/descriptor/test_se_r.pysource/tests/consistent/descriptor/test_se_t.pysource/tests/consistent/descriptor/test_se_t_tebd.pysource/tests/consistent/fitting/test_dipole.pysource/tests/consistent/fitting/test_dos.pysource/tests/consistent/fitting/test_dpa4_ener.pysource/tests/consistent/fitting/test_ener.pysource/tests/consistent/fitting/test_polar.pysource/tests/consistent/fitting/test_property.pysource/tests/consistent/loss/test_dos.pysource/tests/consistent/loss/test_ener.pysource/tests/consistent/loss/test_ener_spin.pysource/tests/consistent/loss/test_property.pysource/tests/consistent/loss/test_tensor.pysource/tests/consistent/model/test_dipole.pysource/tests/consistent/model/test_dos.pysource/tests/consistent/model/test_dpa1.pysource/tests/consistent/model/test_ener.pysource/tests/consistent/model/test_frozen.pysource/tests/consistent/model/test_linear_ener.pysource/tests/consistent/model/test_polar.pysource/tests/consistent/model/test_property.pysource/tests/consistent/model/test_zbl_ener.pysource/tests/consistent/test_activation.pysource/tests/consistent/test_learning_rate.pysource/tests/consistent/test_type_embedding.py
Comment on lines
+58
to
+64
| DPA1_ENER_CURATED_CASES = ( | ||
| ("strip", False), | ||
| ("concat", False), | ||
| ) | ||
|
|
||
|
|
||
| @parameterized_cases(*DPA1_ENER_CURATED_CASES) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Restore smooth_type_embedding=True coverage.
This curated list drops that branch entirely, so this model-level suite no longer exercises a descriptor mode that is still wired into data().
Proposed fix
DPA1_ENER_CURATED_CASES = (
("strip", False),
+ ("strip", True),
("concat", False),
+ ("concat", True),
)📝 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_ENER_CURATED_CASES = ( | |
| ("strip", False), | |
| ("concat", False), | |
| ) | |
| @parameterized_cases(*DPA1_ENER_CURATED_CASES) | |
| DPA1_ENER_CURATED_CASES = ( | |
| ("strip", False), | |
| ("strip", True), | |
| ("concat", False), | |
| ("concat", True), | |
| ) | |
| `@parameterized_cases`(*DPA1_ENER_CURATED_CASES) |
🤖 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/consistent/model/test_dpa1.py` around lines 58 - 64, The curated
DPA1_ENER test cases no longer cover the smooth-type-embedding branch, so
restore that configuration in DPA1_ENER_CURATED_CASES and keep the related model
coverage in test_dpa1.py. Update the parameterized_cases setup so the suite
still exercises the descriptor mode wired through data(), using the existing
DPA1_ENER_CURATED_CASES and test_dpa1 symbols to place the new case consistently
with the current matrix.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5606 +/- ##
==========================================
- Coverage 82.37% 82.35% -0.02%
==========================================
Files 902 902
Lines 101529 101528 -1
Branches 4058 4058
==========================================
- Hits 83630 83614 -16
- Misses 16434 16451 +17
+ Partials 1465 1463 -2 ☔ View full report in Codecov by Harness. 🚀 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
Tests
Summary by CodeRabbit