Implement Wiener Filter #548
Conversation
WalkthroughAdds a new patch-level Wiener filter and re-exports it; centralizes per-dimension window-size logic via Changes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/test_proc/test_wiener.py (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
🔇 Additional comments (19)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #548 +/- ##
=======================================
Coverage 99.92% 99.92%
=======================================
Files 125 126 +1
Lines 10435 10455 +20
=======================================
+ Hits 10427 10447 +20
Misses 8 8
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
dascore/utils/patch.py (2)
628-633: Clarify error message and tailor to allow_multipleMessage reads “one dimension name(s)”. Make it precise and reflect allow_multiple for better UX.
Apply this diff:
- msg = ( - "You must specify one dimension name(s) in args or kwargs. " - f"You passed the following kwargs: {kwargs} args: {args} " - f"to a patch with dimensions {patch.dims}" - ) + expect = "at least one" if allow_multiple else "exactly one" + msg = ( + f"You must specify {expect} dimension name in args or kwargs. " + f"You passed the following kwargs: {kwargs} args: {args} " + f"to a patch with dimensions {patch.dims}" + )
676-753: Optional: cap windows that exceed coordinate length (guard rails)Consider an opt-in enforce_lt_coord to reject windows larger than coord length (using Coord.get_sample_count(..., enforce_lt_coord=True)). Keeps surprises down in very small arrays.
Example diff:
-def get_patch_window_size( +def get_patch_window_size( patch: PatchType, kwargs: dict, samples: bool = False, *, require_odd: bool = False, warn_above: int | None = None, min_samples: int = 1, + enforce_lt_coord: bool = False, ) -> tuple[int, ...]: @@ - samps = coord.get_sample_count(val, samples=samples) + samps = coord.get_sample_count( + val, samples=samples, enforce_lt_coord=enforce_lt_coord + )dascore/proc/hampel.py (1)
138-149: Avoid integer median quantization; compute med/MAD in floatRunning median/MAD on integer arrays can truncate medians and subtly change thresholds. Do calculations in float, then cast final result back.
Apply this diff:
- data = patch.data + data = patch.data @@ - if separable and sum(s > 1 for s in size) >= 2: + dataf = data.astype(np.float64, copy=False) + if separable and sum(s > 1 for s in size) >= 2: @@ - med = _separable_median(data, size, mode) - abs_med_diff = np.abs(data - med) + med = _separable_median(dataf, size, mode) + abs_med_diff = np.abs(dataf - med) mad = _separable_median(abs_med_diff, size, mode) else: - med, abs_med_diff, mad = _calculate_standard_median_and_mad(data, size, mode) + med, abs_med_diff, mad = _calculate_standard_median_and_mad(dataf, size, mode) @@ - out = np.where(thresholded > threshold, med, data) + out = np.where(thresholded > threshold, med, dataf) out = out.astype(data.dtype, copy=False)Also applies to: 143-144
tests/test_utils/test_patch_utils.py (1)
812-814: Avoid catching broad Exception in testsAssert specific exceptions to prevent masking real failures.
Apply these diffs:
- with pytest.raises(Exception): # Should raise during get_dim_axis_value + with pytest.raises(PatchCoordinateError): get_patch_window_size(simple_patch, {"invalid_dim": 5})- with pytest.raises(Exception): # Should raise during require_evenly_sampled + with pytest.raises(CoordError): get_patch_window_size(irregular_patch, {"time": 0.5})Also add the missing import (outside this hunk):
from dascore.exceptions import CoordError # at the existing exceptions importAlso applies to: 827-829
tests/test_proc/test_wiener.py (1)
190-195: Remove unused fixture argumentnoisy_patch isn’t used in this test.
Apply this diff:
- def test_noise_reduction_effectiveness(self, noisy_patch): + def test_noise_reduction_effectiveness(self):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
dascore/core/patch.py(1 hunks)dascore/proc/__init__.py(1 hunks)dascore/proc/hampel.py(2 hunks)dascore/proc/wiener.py(1 hunks)dascore/utils/patch.py(3 hunks)tests/test_proc/test_filter.py(3 hunks)tests/test_proc/test_hampel.py(0 hunks)tests/test_proc/test_wiener.py(1 hunks)tests/test_utils/test_patch_utils.py(4 hunks)
💤 Files with no reviewable changes (1)
- tests/test_proc/test_hampel.py
🧰 Additional context used
🧬 Code graph analysis (8)
tests/test_proc/test_wiener.py (5)
dascore/exceptions.py (1)
ParameterError(26-27)dascore/examples.py (1)
get_example_patch(659-706)dascore/core/patch.py (2)
data(237-239)Patch(28-442)dascore/proc/wiener.py (1)
wiener_filter(14-74)dascore/core/coordmanager.py (1)
step(1036-1038)
dascore/proc/hampel.py (1)
dascore/utils/patch.py (1)
get_patch_window_size(676-752)
tests/test_proc/test_filter.py (3)
dascore/exceptions.py (1)
ParameterError(26-27)dascore/proc/filter.py (2)
median_filter(201-249)savgol_filter(321-378)dascore/core/patch.py (2)
Patch(28-442)data(237-239)
dascore/proc/wiener.py (1)
dascore/utils/patch.py (2)
get_patch_window_size(676-752)patch_function(180-286)
dascore/core/patch.py (1)
dascore/proc/wiener.py (1)
wiener_filter(14-74)
dascore/proc/__init__.py (1)
dascore/proc/wiener.py (1)
wiener_filter(14-74)
tests/test_utils/test_patch_utils.py (4)
dascore/utils/patch.py (1)
get_patch_window_size(676-752)dascore/exceptions.py (1)
ParameterError(26-27)dascore/examples.py (1)
get_example_patch(659-706)dascore/proc/coords.py (1)
get_coord(87-139)
dascore/utils/patch.py (4)
dascore/core/patch.py (4)
dims(212-214)size(247-249)data(237-239)ndim(217-219)dascore/exceptions.py (1)
ParameterError(26-27)dascore/core/coords.py (5)
size(473-475)data(606-608)ndim(468-470)get_coord(1515-1703)get_sample_count(753-789)dascore/proc/coords.py (1)
get_coord(87-139)
🪛 Ruff (0.13.1)
tests/test_proc/test_wiener.py
190-190: Unused method argument: noisy_patch
(ARG002)
tests/test_utils/test_patch_utils.py
812-812: Do not assert blind exception: Exception
(B017)
827-827: Do not assert blind exception: Exception
(B017)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (windows-latest, 3.13)
- GitHub Check: test_code (macos-latest, 3.10)
- GitHub Check: test_code (macos-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.13)
- GitHub Check: test_code_min_deps (windows-latest, 3.12)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
- GitHub Check: test_code_min_deps (windows-latest, 3.13)
- GitHub Check: test_code_min_deps (macos-latest, 3.12)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.13)
- GitHub Check: test_code_min_deps (macos-latest, 3.13)
🔇 Additional comments (5)
dascore/utils/patch.py (1)
635-637: Consistent exception type for input validation paths?Ambiguous/missing dims raise ParameterError, while extra inputs raise PatchCoordinateError. If intentional, fine; otherwise consider standardizing on ParameterError for user input issues.
dascore/proc/hampel.py (1)
131-133: Good centralization of window sizingSwitching to get_patch_window_size simplifies validation and makes behavior consistent across filters.
dascore/proc/__init__.py (1)
18-19: Expose wiener_filter in proc packagePublic re-export looks good and aligns with Patch method wiring.
dascore/core/patch.py (1)
349-350: Patch method alias for wiener_filterBinding is consistent with other filters. No issues.
dascore/proc/wiener.py (1)
72-74: Thin wrapper is correct; consider dtype expectationsscipy.signal.wiener returns float; downstream code expecting float is fine, but confirm this matches other filters’ dtype behavior. If you need integer-preserving behavior, cast at the very end (with care re: precision).
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
dascore/proc/wiener.py (1)
80-81: Consider warning on overly large windows for parity with Hampel.Using warn_above mirrors Hampel’s UX and prevents accidental slow configs.
Apply this diff:
- size = get_patch_window_size(patch, kwargs, samples, min_samples=1) + size = get_patch_window_size(patch, kwargs, samples, min_samples=1, warn_above=10)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
dascore/proc/hampel.py(3 hunks)dascore/proc/wiener.py(1 hunks)dascore/utils/patch.py(3 hunks)tests/test_proc/test_filter.py(4 hunks)tests/test_proc/test_hampel.py(1 hunks)tests/test_proc/test_wiener.py(1 hunks)tests/test_utils/test_patch_utils.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_proc/test_filter.py
🧰 Additional context used
🧬 Code graph analysis (6)
dascore/proc/wiener.py (2)
dascore/exceptions.py (1)
ParameterError(26-27)dascore/utils/patch.py (2)
get_patch_window_size(677-762)patch_function(180-286)
tests/test_proc/test_hampel.py (2)
dascore/core/patch.py (2)
data(237-239)dtype(252-254)dascore/proc/hampel.py (1)
hampel_filter(39-157)
tests/test_proc/test_wiener.py (4)
dascore/exceptions.py (1)
ParameterError(26-27)dascore/examples.py (1)
get_example_patch(659-706)dascore/core/patch.py (2)
data(237-239)Patch(28-442)dascore/proc/wiener.py (1)
wiener_filter(15-82)
dascore/utils/patch.py (3)
dascore/exceptions.py (1)
ParameterError(26-27)dascore/core/coords.py (3)
ndim(468-470)get_coord(1515-1703)get_sample_count(753-789)dascore/proc/coords.py (1)
get_coord(87-139)
tests/test_utils/test_patch_utils.py (4)
dascore/exceptions.py (2)
CoordError(38-39)ParameterError(26-27)dascore/utils/patch.py (1)
get_patch_window_size(677-762)dascore/examples.py (1)
get_example_patch(659-706)dascore/proc/coords.py (2)
update_coords(218-243)get_coord(87-139)
dascore/proc/hampel.py (2)
dascore/utils/patch.py (1)
get_patch_window_size(677-762)dascore/core/patch.py (3)
size(247-249)data(237-239)dtype(252-254)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: test_code (windows-latest, 3.10)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (windows-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (macos-latest, 3.12)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: test_code (ubuntu-latest, 3.13)
- GitHub Check: test_code (macos-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- GitHub Check: test_code (macos-latest, 3.13)
- GitHub Check: test_code_min_deps (windows-latest, 3.13)
- GitHub Check: test_code_min_deps (windows-latest, 3.12)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
- GitHub Check: test_code_min_deps (macos-latest, 3.12)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.13)
- GitHub Check: test_code_min_deps (macos-latest, 3.13)
🔇 Additional comments (10)
tests/test_utils/test_patch_utils.py (5)
207-209: Good change: error type/message clarified for missing dim.Switching to ParameterError with the “You must specify …” message aligns with utils.get_dim_axis_value behavior.
239-241: Good change: consistent error for multiple dims when not allowed.ParameterError with the same message pattern keeps expectations consistent.
763-777: Odd-window handling tests look solid.Covers both samples=False auto-bump and samples=True hard error paths.
785-804: Nice coverage for performance warnings.Warn threshold behavior and the no-warning path are both exercised.
816-829: Even sampling requirement is validated correctly.Expecting CoordError for non-even sampling matches patch.get_coord(require_evenly_sampled=True).
tests/test_proc/test_hampel.py (1)
391-397: Integer dtype preservation test is valuable.Confirms round-and-cast behavior without over-constraining values.
dascore/proc/hampel.py (2)
131-137: Centralized window sizing via get_patch_window_size is the right move.require_odd=True, min_samples=3, and warn_above=10 encode sensible defaults and simplify validation.
153-157: Safe dtype restoration.Using float64 for ops, rint for integer inputs, then casting back avoids overflow and preserves original dtype.
tests/test_proc/test_wiener.py (1)
103-107: Test expectation depends on capitalized “Must specify”.With the proposed wiener_filter message change, this will pass.
If the message is adjusted differently, update the regex here accordingly.
dascore/utils/patch.py (1)
629-635: Improved guidance in “You must specify …” message.Clearer direction for callers; aligns with updated tests.
| if not kwargs: | ||
| msg = ( | ||
| "To use wiener_filter you must specify dimension-specific window " | ||
| "sizes via kwargs (e.g., time=5, distance=3)" | ||
| ) | ||
| raise ParameterError(msg) | ||
|
|
There was a problem hiding this comment.
Fix error-message to satisfy test expectation.
tests/test_proc/test_wiener.py expects “Must specify …”. Current message is lower‑case and will fail the regex.
Apply this diff:
@@
- if not kwargs:
- msg = (
- "To use wiener_filter you must specify dimension-specific window "
- "sizes via kwargs (e.g., time=5, distance=3)"
- )
- raise ParameterError(msg)
+ if not kwargs:
+ msg = (
+ "Must specify at least one dimension-specific window size via kwargs "
+ "(e.g., time=5, distance=3) to use wiener_filter."
+ )
+ raise ParameterError(msg)📝 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.
| if not kwargs: | |
| msg = ( | |
| "To use wiener_filter you must specify dimension-specific window " | |
| "sizes via kwargs (e.g., time=5, distance=3)" | |
| ) | |
| raise ParameterError(msg) | |
| if not kwargs: | |
| msg = ( | |
| "Must specify at least one dimension-specific window size via kwargs " | |
| "(e.g., time=5, distance=3) to use wiener_filter." | |
| ) | |
| raise ParameterError(msg) |
🤖 Prompt for AI Agents
In dascore/proc/wiener.py around lines 73 to 79, the raised ParameterError
message starts with a lowercase "To use..." but tests expect the message to
begin with "Must specify ...". Update the message string so it starts with "Must
specify" and otherwise conveys the same guidance (e.g., "Must specify
dimension-specific window sizes via kwargs (e.g., time=5, distance=3)"). Ensure
the new message replaces the existing msg variable before raising
ParameterError.
| msg = f"The following input dimensions are not found in the patch. {remaining}" | ||
| raise PatchCoordinateError(msg) | ||
| # Ensure order is preserved (eg args, then kwargs) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Leave PatchCoordinateError here; normalize at the window API boundary.
Keeping this as PatchCoordinateError preserves existing behaviors. The re-raise in get_patch_window_size will satisfy new tests without breaking others.
🤖 Prompt for AI Agents
In dascore/utils/patch.py around lines 637 to 639, do not change the current
PatchCoordinateError raise here — keep raising PatchCoordinateError with the
message about missing input dimensions so existing behavior remains; instead,
update the window API boundary (specifically get_patch_window_size) to catch
this PatchCoordinateError and normalize/translate it there (re-raise or convert
to the new expected error form at that boundary) so new tests pass without
altering behavior elsewhere.
| def get_patch_window_size( | ||
| patch: PatchType, | ||
| kwargs: dict, | ||
| samples: bool = False, | ||
| *, | ||
| require_odd: bool = False, | ||
| warn_above: int | None = None, | ||
| min_samples: int = 1, | ||
| enforce_lt_coord: bool = False, | ||
| ) -> tuple[int, ...]: | ||
| """ | ||
| Get window sizes for patch processing operations. | ||
| Parameters | ||
| ---------- | ||
| patch | ||
| The input patch. | ||
| kwargs | ||
| Keyword arguments specifying dimension names and their window sizes. | ||
| samples | ||
| If True, kwargs values are in samples; if False, in coordinate units. | ||
| require_odd | ||
| If True, require odd window sizes. When samples=False, even sizes | ||
| are adjusted to be odd. When samples=True, even sizes raise ParameterError. | ||
| warn_above | ||
| If specified, warn when any dimension window size exceeds this value. | ||
| min_samples | ||
| Minimum number of samples required per dimension. | ||
| enforce_lt_coord | ||
| If True, reject windows larger than coordinate length. | ||
| Returns | ||
| ------- | ||
| Tuple of window sizes for each dimension of the patch data. | ||
| Raises | ||
| ------ | ||
| ParameterError | ||
| If window sizes are too small, or if require_odd=True and samples=True | ||
| but window size is even. | ||
| """ | ||
| # Handle empty kwargs case - return all ones | ||
| if not kwargs: | ||
| return tuple([1] * patch.data.ndim) | ||
|
|
||
| aggs = get_dim_axis_value(patch, kwargs=kwargs, allow_multiple=True) | ||
| size = [1] * patch.data.ndim | ||
|
|
||
| for name, axis, val in aggs: | ||
| coord = patch.get_coord(name, require_evenly_sampled=True) | ||
| samps = coord.get_sample_count( | ||
| val, samples=samples, enforce_lt_coord=enforce_lt_coord | ||
| ) | ||
|
|
||
| # Check minimum samples requirement | ||
| if samps < min_samples: | ||
| msg = ( | ||
| f"Window must have at least {min_samples} samples along each " | ||
| f"dimension. {name} has {samps} samples. Try increasing its value." | ||
| ) | ||
| raise ParameterError(msg) | ||
|
|
||
| # Handle odd number requirement | ||
| if require_odd and (samps % 2 != 1): | ||
| if not samples: | ||
| # Adjust even sizes to odd when samples=False | ||
| samps += 1 | ||
| else: | ||
| # Raise error when samples=True and size is even | ||
| msg = ( | ||
| f"For clean median calculation, dimension windows must be odd " | ||
| f"but {name} has a value of {samps} samples." | ||
| ) | ||
| raise ParameterError(msg) | ||
|
|
||
| # Issue warning for large window sizes | ||
| if warn_above is not None and samps > warn_above: | ||
| msg = ( | ||
| f"Large window size ({samps} samples) in dimension '{name}' " | ||
| f"may result in slow performance. Consider reducing the window size." | ||
| ) | ||
| warnings.warn(msg, UserWarning, stacklevel=3) | ||
|
|
||
| size[axis] = samps | ||
|
|
||
| return tuple(size) | ||
|
|
There was a problem hiding this comment.
get_patch_window_size: add normalization of invalid-dim errors.
To satisfy new tests (invalid-dim → ParameterError) and keep lower-level semantics intact, wrap get_dim_axis_value in try/except and re-raise ParameterError. See suggested diff in tests/test_utils/test_patch_utils.py comment.
Additionally, minor doc nit: mention that when samples=True, kwargs values must be integers (enforced downstream).
🤖 Prompt for AI Agents
In dascore/utils/patch.py around lines 677 to 763, wrap the
get_dim_axis_value(...) call in a try/except that catches the library's
invalid-dim exception (e.g., InvalidDimError or whatever specific exception that
function raises), and re-raise a ParameterError with the same message (preserve
the original exception as the cause if desired) so tests expecting invalid-dim →
ParameterError pass; also update the function docstring to note that when
samples=True the kwargs values must be integers (this is enforced downstream).
| def test_invalid_dimension_raises(self, simple_patch): | ||
| """Test invalid dimension name raises error.""" | ||
| with pytest.raises(ParameterError): | ||
| get_patch_window_size(simple_patch, {"invalid_dim": 5}) | ||
|
|
There was a problem hiding this comment.
Mismatch: get_patch_window_size raises PatchCoordinateError on invalid dims.
This test expects ParameterError, but get_dim_axis_value currently raises PatchCoordinateError for extra/unknown dims and get_patch_window_size doesn’t normalize it. Wrap and re-raise as ParameterError in get_patch_window_size.
Apply this diff in dascore/utils/patch.py to normalize the exception:
@@
- aggs = get_dim_axis_value(patch, kwargs=kwargs, allow_multiple=True)
+ try:
+ aggs = get_dim_axis_value(patch, kwargs=kwargs, allow_multiple=True)
+ except PatchCoordinateError as e:
+ # Normalize invalid/unknown dimension errors for window-sizing API.
+ raise ParameterError(str(e)) from e📝 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.
| def test_invalid_dimension_raises(self, simple_patch): | |
| """Test invalid dimension name raises error.""" | |
| with pytest.raises(ParameterError): | |
| get_patch_window_size(simple_patch, {"invalid_dim": 5}) | |
| try: | |
| aggs = get_dim_axis_value(patch, kwargs=kwargs, allow_multiple=True) | |
| except PatchCoordinateError as e: | |
| # Normalize invalid/unknown dimension errors for window-sizing API. | |
| raise ParameterError(str(e)) from e |
🤖 Prompt for AI Agents
In dascore/utils/patch.py around the get_patch_window_size function,
get_patch_window_size currently lets PatchCoordinateError bubble up when
unknown/extra dimension names are passed; update the function to catch
PatchCoordinateError raised during dimension normalization/lookup and re-raise
it as a ParameterError (preserving the original error message or including it)
so callers expecting ParameterError get a consistent exception; ensure other
exceptions are not swallowed and maintain existing return behavior.
Description
This PR implements a thin patch wrapper around scipy's wiener filter. It also refactors some internals.
Checklist
I have (if applicable):
Summary by CodeRabbit
New Features
Refactor
Tests