Skip to content

Add AlphaMode enum for stringly-typed alpha_mode field#705

Merged
waltsims merged 3 commits into
masterfrom
cleanup-alpha-mode-enum
May 3, 2026
Merged

Add AlphaMode enum for stringly-typed alpha_mode field#705
waltsims merged 3 commits into
masterfrom
cleanup-alpha-mode-enum

Conversation

@waltsims
Copy link
Copy Markdown
Owner

@waltsims waltsims commented Apr 1, 2026

  • AlphaMode(str, Enum) with NO_ABSORPTION, NO_DISPERSION, STOKES
  • kWaveMedium.post_init normalizes string inputs to enum
  • Backward compatible: AlphaMode == "no_dispersion" works via str inheritance
  • str returns the value so f-strings render cleanly

Greptile Summary

Adds AlphaMode(str, Enum) to replace the stringly-typed alpha_mode field and introduces a _to_alpha_mode helper that normalises the value at construction time (__post_init__) and again inside check_fields to cover post-construction string reassignment. Because AlphaMode inherits from str, all existing equality/membership comparisons across the codebase (checks.py, create_absorption_variables.py, kWaveSimulation.py) continue to work without modification.

Confidence Score: 5/5

Safe to merge — only P2 style findings, no logic or correctness issues.

All three changed files are correct. The str-Enum design preserves backward compatibility with every existing string comparison in the codebase. The two previous review concerns (post-construction bypass, unhelpful error message) are both addressed. Only minor style findings remain.

No files require special attention.

Important Files Changed

Filename Overview
kwave/enums.py Adds AlphaMode(str, Enum) with three members and an explicit str that returns self.value for clean f-string rendering.
kwave/kmedium.py Introduces _to_alpha_mode helper, normalizes alpha_mode in post_init and check_fields, handles post-construction string reassignment; field comment omits AlphaMode.STOKES.
tests/test_kmedium.py New test file covering construction normalization, invalid-input errors, post-construction string assignment, and backward-compatible string comparisons.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["caller sets alpha_mode\n(str, AlphaMode, or None)"] --> B{__post_init__ or\ncheck_fields}
    B --> C["_to_alpha_mode(value)"]
    C --> D{value is None or\nalready AlphaMode?}
    D -- Yes --> E["return as-is"]
    D -- No --> F["AlphaMode(value)"]
    F -- valid string --> G["AlphaMode enum member\n(NO_ABSORPTION / NO_DISPERSION / STOKES)"]
    F -- invalid --> H["ValueError with\nfriendly message"]
    G --> I["self.alpha_mode = AlphaMode member"]
    E --> I
    I --> J["Downstream comparisons\nalpha_mode == 'no_dispersion' ✓\nalpha_mode == 'stokes' ✓\nalpha_mode in list ✓"]
Loading

Reviews (3): Last reviewed commit: "Accept post-construction string assignme..." | Re-trigger Greptile

waltsims and others added 2 commits April 1, 2026 01:36
- AlphaMode(str, Enum) with NO_ABSORPTION, NO_DISPERSION, STOKES
- kWaveMedium.__post_init__ normalizes string inputs to enum
- Backward compatible: AlphaMode == "no_dispersion" works via str inheritance
- __str__ returns the value so f-strings render cleanly

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comment thread kwave/kmedium.py Outdated
Comment on lines +61 to +65
if self.alpha_mode is not None and not isinstance(self.alpha_mode, AlphaMode):
raise ValueError(
f"medium.alpha_mode must be an AlphaMode enum value or one of "
f"'no_absorption', 'no_dispersion', 'stokes', got {self.alpha_mode!r}"
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Post-construction string assignment silently breaks check_fields

check_fields is called from kWaveSimulation after the object is fully constructed (line 610 in kWaveSimulation.py). Since kWaveMedium is a plain dataclass, its attributes are mutable — a caller can legally write medium.alpha_mode = "no_dispersion" after __post_init__ runs, bypassing the normalization step. Under the old code that assignment would still pass the in [...] check; under this PR it raises a ValueError even for a valid string value, silently breaking previously-working patterns.

The type hint Optional[Union[AlphaMode, str]] reinforces the expectation that plain strings remain valid inputs throughout the object's lifetime. Fix by normalising strings in check_fields as well, or by converting alpha_mode to a property setter so normalisation is always applied on write:

# check the absorption mode input is valid (already normalized to AlphaMode in __post_init__)
if self.alpha_mode is not None:
    if isinstance(self.alpha_mode, str) and not isinstance(self.alpha_mode, AlphaMode):
        self.alpha_mode = AlphaMode(self.alpha_mode)  # late-normalise post-construction strings
    elif not isinstance(self.alpha_mode, AlphaMode):
        raise ValueError(
            f"medium.alpha_mode must be an AlphaMode enum value or one of "
            f"'no_absorption', 'no_dispersion', 'stokes', got {self.alpha_mode!r}"
        )

Comment thread kwave/kmedium.py Outdated
Comment on lines +47 to +48
if isinstance(self.alpha_mode, str) and not isinstance(self.alpha_mode, AlphaMode):
self.alpha_mode = AlphaMode(self.alpha_mode)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Invalid string raises unhelpful ValueError from enum constructor

When an invalid string is passed at construction time (e.g. kWaveMedium(sound_speed=1500, alpha_mode="typo")), AlphaMode("typo") raises ValueError: 'typo' is not a valid AlphaMode — the more descriptive message in check_fields is never reached. Consider wrapping the conversion to re-raise with a clearer hint:

if isinstance(self.alpha_mode, str) and not isinstance(self.alpha_mode, AlphaMode):
    try:
        self.alpha_mode = AlphaMode(self.alpha_mode)
    except ValueError:
        valid = [m.value for m in AlphaMode]
        raise ValueError(
            f"medium.alpha_mode must be one of {valid}, got {self.alpha_mode!r}"
        )

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 1, 2026

Codecov Report

❌ Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 74.46%. Comparing base (ba553eb) to head (0a8ae37).
⚠️ Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
kwave/enums.py 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #705      +/-   ##
==========================================
+ Coverage   74.40%   74.46%   +0.05%     
==========================================
  Files          56       56              
  Lines        8026     8040      +14     
  Branches     1570     1570              
==========================================
+ Hits         5972     5987      +15     
  Misses       1437     1437              
+ Partials      617      616       -1     
Flag Coverage Δ
3.10 74.46% <94.44%> (+0.05%) ⬆️
3.11 74.46% <94.44%> (+0.05%) ⬆️
3.12 74.46% <94.44%> (+0.05%) ⬆️
3.13 74.46% <94.44%> (+0.05%) ⬆️
macos-latest 74.44% <94.44%> (+0.05%) ⬆️
ubuntu-latest 74.44% <94.44%> (+0.05%) ⬆️
windows-latest 74.45% <94.44%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

The type hint advertises Union[AlphaMode, str], so a plain string
assigned after __post_init__ runs (e.g. medium.alpha_mode = "no_dispersion")
must remain valid. check_fields previously rejected this. Extracts a
_to_alpha_mode helper used by both __post_init__ and check_fields, which
also gives a friendly error message for invalid strings instead of the
bare ValueError from AlphaMode("garbage").

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@waltsims
Copy link
Copy Markdown
Owner Author

waltsims commented May 3, 2026

@Greptile-app

@waltsims waltsims merged commit 293f16e into master May 3, 2026
33 checks passed
@waltsims waltsims deleted the cleanup-alpha-mode-enum branch May 3, 2026 18:42
waltsims added a commit that referenced this pull request May 16, 2026
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>
@waltsims waltsims mentioned this pull request May 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant