add adaptive spectral filter#670
Conversation
📝 WalkthroughWalkthroughAdds adaptive spectral filtering for DASCore patches with 1D/2D windowed FFT-based weighting, tapered overlap-add reconstruction, and optional Numba acceleration. Includes dimension resolution, coordinate-to-sample conversion, engine selection, comprehensive tests, and Patch API integration. ChangesAdaptive Spectral Filtering Implementation and Integration
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@dascore/proc/adaptive_spectral_filter.py`:
- Around line 79-85: The isinstance checks in the loop over window_size and
overlap use tuple syntax which Ruff disallows; update both checks to use union
type syntax: change isinstance(window, (int, np.integer)) to isinstance(window,
int | np.integer) and isinstance(axis_overlap, (int, np.integer)) to
isinstance(axis_overlap, int | np.integer) in the loop that iterates over axis,
(window, axis_overlap) (variables: window_size, overlap, window, axis_overlap,
axis).
In `@docs/references.bib`:
- Around line 71-78: Update the BibTeX record for isken2022denoising to include
missing volume and issue and correct the pages: add volume={231}, issue={2}, and
replace pages={ggac229} with the actual page range pages={944--949} while
keeping the rest of the fields (title, author, journal, year, doi) unchanged.
In `@tests/test_proc/test_adaptive_spectral_filter.py`:
- Around line 27-33: The default argument for time_step in _patch uses a runtime
call np.timedelta64(4, "ms") which triggers Ruff B008; change the signature to
accept time_step: Optional[...] = None (or a sentinel) and inside the _patch
function set time_step = np.timedelta64(4, "ms") if time_step is None, ensuring
the np.timedelta64 construction happens at call time rather than at function
definition time.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a69e9e8b-b37b-4392-adad-4ff01815a878
📒 Files selected for processing (9)
dascore/core/patch.pydascore/proc/__init__.pydascore/proc/_adaptive_spectral_filter_numba.pydascore/proc/adaptive_spectral_filter.pydascore/utils/signal.pydocs/references.bibpyproject.tomltests/test_proc/test_adaptive_spectral_filter.pytests/test_proc/test_taper.py
| for axis, (window, axis_overlap) in enumerate(zip(window_size, overlap)): | ||
| if not isinstance(window, (int, np.integer)): | ||
| msg = f"window_size[{axis}] must be an integer; got {window!r}." | ||
| raise ValueError(msg) | ||
| if not isinstance(axis_overlap, (int, np.integer)): | ||
| msg = f"overlap[{axis}] must be an integer; got {axis_overlap!r}." | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
Fix isinstance syntax to resolve pipeline failure.
The CI is failing because Ruff UP038 requires using X | Y union syntax in isinstance calls instead of tuple syntax (X, Y).
Proposed fix
for axis, (window, axis_overlap) in enumerate(zip(window_size, overlap)):
- if not isinstance(window, (int, np.integer)):
+ if not isinstance(window, int | np.integer):
msg = f"window_size[{axis}] must be an integer; got {window!r}."
raise ValueError(msg)
- if not isinstance(axis_overlap, (int, np.integer)):
+ if not isinstance(axis_overlap, int | np.integer):
msg = f"overlap[{axis}] must be an integer; got {axis_overlap!r}."
raise ValueError(msg)🧰 Tools
🪛 GitHub Actions: LintCode / 0_lint_code.txt
[error] 80-80: ruff (hook id: ruff) failed: UP038 Use X | Y in isinstance call instead of (X, Y)
[error] 83-83: ruff (hook id: ruff) failed: UP038 Use X | Y in isinstance call instead of (X, Y)
🪛 GitHub Actions: LintCode / lint_code
[error] 80-83: ruff (hook id: ruff) failed with exit code 1. UP038: Use X | Y in isinstance call instead of (X, Y) (2 occurrences).
🪛 Ruff (0.15.12)
[warning] 79-79: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 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 `@dascore/proc/adaptive_spectral_filter.py` around lines 79 - 85, The
isinstance checks in the loop over window_size and overlap use tuple syntax
which Ruff disallows; update both checks to use union type syntax: change
isinstance(window, (int, np.integer)) to isinstance(window, int | np.integer)
and isinstance(axis_overlap, (int, np.integer)) to isinstance(axis_overlap, int
| np.integer) in the loop that iterates over axis, (window, axis_overlap)
(variables: window_size, overlap, window, axis_overlap, axis).
| @article{isken2022denoising, | ||
| title={De-noising distributed acoustic sensing data using an adaptive frequency-wavenumber filter}, | ||
| author={Isken, Marius Paul and Vasyura-Bathke, Hannes and Dahm, Torsten and Heimann, Sebastian}, | ||
| journal={Geophysical Journal International}, | ||
| pages={ggac229}, | ||
| year={2022}, | ||
| doi={10.1093/gji/ggac229} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What are the correct bibliographic details for DOI 10.1093/gji/ggac229?
💡 Result:
The bibliographic details for the article associated with DOI 10.1093/gji/ggac229 are as follows: Authors: Isken, M. P., Vasyura-Bathke, H., Dahm, T., and Heimann, S. [1] Title: De-noising distributed acoustic sensing data using an adaptive frequency–wavenumber filter [1] Journal: Geophysical Journal International [2][1] Volume: 231 [2][1] Issue: 2 [2][1] Pages: 944–949 [2][1] Publication Date: 17 June 2022 (Advance Access) [3][2] DOI: 10.1093/gji/ggac229 [2][1]
Citations:
- 1: https://gfzpublic.gfz.de/rest/items/item_5012128_3/component/file_5012712/content
- 2: https://academic.oup.com/gji/article/231/2/944/6609779
- 3: https://academic.oup.com/gji/article-pdf/231/2/944/45054596/ggac229.pdf
Add volume, issue, and correct page numbers to the BibTeX entry.
The DOI resolves correctly and most metadata matches the published article. However, the entry is missing the volume (231) and issue (2) numbers, and the pages field contains the article ID (ggac229) instead of the actual page range (944–949). Update the entry:
Corrected BibTeX entry
`@article`{isken2022denoising,
title={De-noising distributed acoustic sensing data using an adaptive frequency-wavenumber filter},
author={Isken, Marius Paul and Vasyura-Bathke, Hannes and Dahm, Torsten and Heimann, Sebastian},
journal={Geophysical Journal International},
volume={231},
issue={2},
pages={944--949},
year={2022},
doi={10.1093/gji/ggac229}
}
🤖 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 `@docs/references.bib` around lines 71 - 78, Update the BibTeX record for
isken2022denoising to include missing volume and issue and correct the pages:
add volume={231}, issue={2}, and replace pages={ggac229} with the actual page
range pages={944--949} while keeping the rest of the fields (title, author,
journal, year, doi) unchanged.
| def _patch( | ||
| shape: tuple[int, ...], | ||
| dims: tuple[str, ...], | ||
| *, | ||
| dtype=np.float32, | ||
| time_step=np.timedelta64(4, "ms"), | ||
| distance_step=1.0, |
There was a problem hiding this comment.
Move np.timedelta64 construction out of the default argument (Ruff B008).
Line 32 evaluates a function call at definition time, which can fail lint gates configured with Ruff B008.
💡 Proposed fix
+DEFAULT_TIME_STEP = np.timedelta64(4, "ms")
+
def _patch(
shape: tuple[int, ...],
dims: tuple[str, ...],
*,
dtype=np.float32,
- time_step=np.timedelta64(4, "ms"),
+ time_step=DEFAULT_TIME_STEP,
distance_step=1.0,
) -> dc.Patch:📝 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 _patch( | |
| shape: tuple[int, ...], | |
| dims: tuple[str, ...], | |
| *, | |
| dtype=np.float32, | |
| time_step=np.timedelta64(4, "ms"), | |
| distance_step=1.0, | |
| DEFAULT_TIME_STEP = np.timedelta64(4, "ms") | |
| def _patch( | |
| shape: tuple[int, ...], | |
| dims: tuple[str, ...], | |
| *, | |
| dtype=np.float32, | |
| time_step=DEFAULT_TIME_STEP, | |
| distance_step=1.0, |
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 32-32: Do not perform function call np.timedelta64 in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🤖 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 `@tests/test_proc/test_adaptive_spectral_filter.py` around lines 27 - 33, The
default argument for time_step in _patch uses a runtime call np.timedelta64(4,
"ms") which triggers Ruff B008; change the signature to accept time_step:
Optional[...] = None (or a sentinel) and inside the _patch function set
time_step = np.timedelta64(4, "ms") if time_step is None, ensuring the
np.timedelta64 construction happens at call time rather than at function
definition time.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6db1dd6db9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| msg = f"overlap contains dimensions not being filtered: {sorted(extra)}" | ||
| raise ParameterError(msg) | ||
| return defaults | dict(overlap), frozenset(set(dims) - set(overlap)) | ||
| return {dim: int(overlap) for dim in dims}, frozenset() |
There was a problem hiding this comment.
Do not coerce scalar overlap to int here. When users call adaptive_spectral_filter(..., samples=False, overlap=<unit value>), this path forces overlap into a Python integer before coordinate conversion, which raises for time-like values (e.g., np.timedelta64) and silently changes non-integer unit values. That breaks the documented coordinate-unit behavior for scalar overlap and causes valid 1D/2D calls with unit overlaps to fail before filtering starts.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #670 +/- ##
========================================
Coverage 99.93% 99.93%
========================================
Files 135 137 +2
Lines 11663 11998 +335
========================================
+ Hits 11655 11990 +335
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:
|
|
✅ Documentation built: |
1 participant
Description
This PR adds
Patch.adaptive_spectral_filter, a windowed adaptive spectral filter for DASCore patches.The new method supports:
(f-k)filtering over two selected dimensions, equivalent to the AFK method described by Isken et al. (2022).Checklist
I have (if applicable):
Summary by CodeRabbit
New Features
rocket-fftis installed.Chores
rocket-fftas an optional dependency.