Fix kWaveMedium type annotations (rebased #636 + Greptile fixes)#736
Conversation
Original change (TullyMonster): replace incorrect `np.array` type hints (which is a callable, not a type) with proper `Optional[Union[float, int, np.ndarray]]` annotations across the `kWaveMedium` dataclass. This eliminates IDE warnings like "Expected type 'None', got 'float'" when passing scalars for `alpha_coeff`, `alpha_power`, etc. Also reconciled with the AlphaMode enum work from #705 (kept enum-aware `Optional[Union[AlphaMode, str]]` for `alpha_mode`). Greptile P2 fixes applied on top: - Drop unused `import kwave.utils.checks` (dead code after the `np.issubdtype(...)` migration replaced the `is_number(...)` call). - The new type hint allows `np.ndarray` for `alpha_power`, but `np.isscalar(np.array(1.5))` is `False`. Widen the scalar check in `_check_absorbing_without_stokes` so 0-d arrays are accepted as scalars. Regression test added. Test coverage rolls up existing AlphaMode normalization tests from master with the broader coverage in this PR. Co-Authored-By: TullyMonster <47771282+TullyMonster@users.noreply.github.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # check y is a scalar | ||
| assert np.isscalar(self.alpha_power), "medium.alpha_power must be scalar." | ||
| # check y is a scalar (np.isscalar rejects 0-d ndarrays — accept np.array(1.5) too) | ||
| is_scalar = np.isscalar(self.alpha_power) or (isinstance(self.alpha_power, np.ndarray) and self.alpha_power.size == 1) |
There was a problem hiding this comment.
is_scalar silently accepts 1-D single-element arrays
The condition self.alpha_power.size == 1 admits np.array([1.5]) (a 1-D ndarray), not just a 0-d array. A 1-D array passes the check, then propagates through the remainder of _check_absorbing_without_stokes without being reduced to a true scalar — 0 <= np.array([1.5]) < 3 works at the assertion level due to Python/NumPy single-element coercion, but downstream simulation code that expects a bare scalar for the power-law exponent may see unexpected shapes. Consider extracting the scalar value with .item() once is_scalar has been confirmed, so self.alpha_power is always a Python float at that point.
| def test_elastic_properties_access(): | ||
| """Test access to elastic code related properties (should raise NotImplementedError)""" | ||
| medium = kWaveMedium(sound_speed=1500) | ||
|
|
||
| with pytest.raises(NotImplementedError, match="Elastic simulation"): | ||
| _ = medium.sound_speed_shear | ||
|
|
||
| with pytest.raises(NotImplementedError, match="Elastic simulation"): | ||
| _ = medium.sound_speed_ref_shear | ||
|
|
||
| with pytest.raises(NotImplementedError, match="Elastic simulation"): | ||
| _ = medium.sound_speed_compression | ||
|
|
||
| with pytest.raises(NotImplementedError, match="Elastic simulation"): | ||
| _ = medium.sound_speed_ref_compression | ||
|
|
||
| with pytest.raises(NotImplementedError, match="Elastic simulation"): | ||
| _ = medium.alpha_coeff_compression | ||
|
|
||
| with pytest.raises(NotImplementedError, match="Elastic simulation"): | ||
| _ = medium.alpha_coeff_shear |
There was a problem hiding this comment.
# pragma: no cover markers invalidated by new tests
test_elastic_properties_access directly exercises all six elastic-property stubs (sound_speed_shear, sound_speed_ref_shear, sound_speed_compression, sound_speed_ref_compression, alpha_coeff_compression, alpha_coeff_shear), but each of those properties still carries # pragma: no cover in kmedium.py. Coverage tooling will continue to exclude those lines, making the report misleading. The pragmas can be removed now that the tests cover the stubs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #736 +/- ##
==========================================
+ Coverage 75.04% 75.19% +0.14%
==========================================
Files 57 57
Lines 8128 8132 +4
Branches 1584 1586 +2
==========================================
+ Hits 6100 6115 +15
+ Misses 1405 1395 -10
+ Partials 623 622 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@greptile re-review |
|
@greptile stop |
|
Got it, stopping here. Let me know if you need anything else. |
1 participant
Summary
Rebases @TullyMonster's #636 onto current master and addresses the two open Greptile P2s. Original PR was stalled with merge conflicts after #705 (AlphaMode enum) landed.
Changes from #636 as-is
Optional[Union[AlphaMode, str]]annotation foralpha_mode(master's enum-aware version from Add AlphaMode enum for stringly-typed alpha_mode field #705 supersedes fix: correct type annotations in kWaveMedium class #636'sOptional[str]); merged the new tests with master'sTestAlphaModeNormalizationclass instead of overwriting.import kwave.utils.checks— the oldis_number(...)call was already replaced withnp.issubdtype(...), leaving the import dead.np.ndarrayforalpha_power, butnp.isscalar(np.array(1.5))isFalse, so the scalar check would reject 0-d arrays. Widened the check to accept single-element ndarrays. Regression test (test_absorbing_without_stokes_alpha_power_accepts_0d_ndarray) added.test_alpha_mode_validation_edge_casesnow asserts that empty-stringalpha_modeis rejected at construction (master's_to_alpha_modefrom Add AlphaMode enum for stringly-typed alpha_mode field #705), instead of atcheck_fields.Authorship
The substantive type-annotation work is @TullyMonster's. They're co-authored on the single squashed commit. Closes #636.
Test plan
tests/test_kmedium.pypass locallyruff check .cleanruff format --checkcleanCloses #636
🤖 Generated with Claude Code
Greptile Summary
This PR rebases #636 onto master, fixing type annotations across
kWaveMedium, reconciling with theAlphaModeenum changes from #705, and addressing two previously flagged quality issues (deadkwave.utils.checksimport and the 0-d ndarray scalar check).np.arraystubs replaced with preciseOptional[Union[float, int, np.ndarray]]annotations;check_fieldsparameter updated toSequence[int];alpha_filter/alpha_signnarrowed toOptional[np.ndarray].np.isscalarguard in_check_absorbing_without_stokesnow also accepts single-elementnp.ndarrayvalues (0-d or 1-D), with a regression test added.alpha_signcheck ported tonp.issubdtype;alpha_coeffguard madeNone-safe;logging.log(WARN, ...)replaced withlogging.warning; 27 new or updated tests cover all changed paths.Confidence Score: 5/5
Safe to merge — the changes are scoped to type annotations, dead-code removal, and validation clean-up with no behavioural regressions on existing callers.
All changed validation paths have direct test coverage. The AlphaMode str-enum comparisons are correct throughout, the None-guard added to alpha_coeff in check_fields removes a latent crash, and the 0-d ndarray regression test confirms the widened scalar check works as intended. No path was left unguarded.
No files require special attention.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[kWaveMedium.__post_init__] --> B[np.atleast_1d sound_speed] A --> C[_to_alpha_mode alpha_mode] C --> D{value is None or AlphaMode?} D -- yes --> E[return as-is] D -- no --> F[AlphaMode value str] F -- valid --> G[AlphaMode enum] F -- invalid --> H[ValueError] I[set_absorbing is_absorbing is_stokes] --> J{is_absorbing?} J -- no --> K[set flags False] J -- yes --> L{is_stokes?} L -- yes --> M[_check_absorbing_with_stokes] L -- no --> N[_check_absorbing_without_stokes] M --> M1[ensure alpha_coeff defined] M --> M2{alpha_power set?} M2 -- yes --> M3{size==1 and close to 2?} M3 -- no --> M4[logging.warning] M2 --> M5[overwrite alpha_power=2] M --> M6{alpha_mode in no_absorption/no_dispersion?} M6 -- yes --> M7[NotImplementedError] M --> M8{alpha_filter set?} M8 -- yes --> M9[AssertionError] N --> N1[ensure alpha_coeff + alpha_power defined] N --> N2{isscalar OR ndarray size==1?} N2 -- no --> N3[AssertionError scalar] N2 -- yes --> N4{isreal alpha_coeff AND 0 <= alpha_power < 3?} N4 -- no --> N5[AssertionError range] N4 -- yes --> N6{alpha_mode != no_dispersion AND alpha_power==1?} N6 -- yes --> N7[AssertionError dispersion]Reviews (3): Last reviewed commit: "Merge branch 'master' into chore/636-kme..." | Re-trigger Greptile