Conversation
- Add missing calculate_level=True overloads so mypy resolves explicit calculate_level=True calls correctly (4 clean overloads covering all sigbands × calculate_level combinations) - Fix WeightingFilter multichannel zi bug: lazy allocation deferred to filter() so channel dimension matches actual input shape (same pattern as OctaveFilterBank) - Sync __version__ from '1.1.2' to '1.1.4' to match pyproject.toml - Update WeightingFilter tests for lazy init + add multichannel test
- Change SonarSource/sonarqube-scan-action@master to @v7 (stable) - Replace secrets.TOKEN_GH with secrets.GITHUB_TOKEN (auto-available)
Reviewer's GuideAdds missing type overloads for OctaveFilterBank.filter, fixes multichannel state handling in WeightingFilter via lazy zi initialization, updates tests accordingly, bumps the package version to 1.1.4, and stabilizes the SonarCloud GitHub Action configuration. Sequence diagram for lazy zi initialization in WeightingFilter.filtersequenceDiagram
actor Caller
participant WeightingFilter
participant SciPy_signal
Caller->>WeightingFilter: filter(x)
WeightingFilter->>WeightingFilter: x_proc = _typesignal(x)
WeightingFilter->>WeightingFilter: if not stateful
alt stateful is False
WeightingFilter->>SciPy_signal: sosfilt(sos, x_proc, axis=-1)
SciPy_signal-->>WeightingFilter: y
WeightingFilter-->>Caller: y
else stateful is True
WeightingFilter->>WeightingFilter: n_sections = sos.shape[0]
WeightingFilter->>WeightingFilter: needs_init = zi.size == 0
WeightingFilter->>WeightingFilter: if x_proc is multichannel
alt multichannel and shape mismatch
WeightingFilter->>WeightingFilter: needs_init = True
else no mismatch
WeightingFilter->>WeightingFilter: keep existing zi
end
alt needs_init and x_proc is 1D
WeightingFilter->>WeightingFilter: init zi (n_sections, 2) or steady sosfilt_zi
else needs_init and x_proc is 2D
WeightingFilter->>WeightingFilter: init zi (n_sections, n_channels, 2) or tiled steady zi
end
WeightingFilter->>SciPy_signal: sosfilt(sos, x_proc, axis=-1, zi=zi)
SciPy_signal-->>WeightingFilter: y, zi_new
WeightingFilter->>WeightingFilter: zi = zi_new
WeightingFilter-->>Caller: y
end
Class diagram for updated WeightingFilter and OctaveFilterBank.filterclassDiagram
class WeightingFilter {
- sos: np.ndarray
- stateful: bool
- zi: np.ndarray
- _steady_ic: bool
+ WeightingFilter(fs: int, curve: str, stateful: bool, steady_ic: bool)
+ filter(x: List[float] | np.ndarray) np.ndarray
}
class OctaveFilterBank {
+ filter(x: List[float] | np.ndarray, sigbands: bool, mode: str, detrend: bool, calculate_level: bool) Tuple
}
WeightingFilter ..> OctaveFilterBank: both provide filter functionality
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR updates the package version to 1.1.4, refines GitHub Actions workflow configuration for SonarCloud scanning, updates OctaveFilterBank.filter type hints to include the calculate_level parameter in all overloads, implements lazy initialization of filter state in WeightingFilter, and adds comprehensive tests for multichannel stateful filtering behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves several issues that arose after the introduction of stateful block-wise processing. It enhances type safety by refining method overloads, fixes a critical bug in multichannel filter state management, and ensures the project's version is synchronized. Additionally, it improves the reliability of the continuous integration pipeline. Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/pyoctaveband/parametric_filters.py" line_range="90-99" />
<code_context>
+ # Initialize filter state for stateful block-wise processing.
+ # Uses lazy allocation: zi is sized on first filter() call so that
+ # the channel dimension matches the actual input shape.
if self.stateful:
- if not steady_ic:
- self.zi = np.zeros((self.sos.shape[0], 2))
</code_context>
<issue_to_address>
**issue (bug_risk):** Stateful `zi` shape is not updated when switching from multi-channel to 1D input, which can break `sosfilt`.
The lazy-init logic correctly handles 1D → multi-channel and channel-count changes, but not the reverse. If `zi` was initialized for multi-channel `(n_sections, n_channels, 2)` and a later call uses 1D input, `needs_init` remains `False` and the 3D `zi` is reused.
For 1D `x_proc` with `axis=-1`, `scipy.signal.sosfilt` requires `zi` of shape `(n_sections, 2)`, so this reuse can cause errors or incorrect filtering. Please extend the `needs_init` condition to also cover multi-channel → 1D transitions, e.g. reset when `self.zi.ndim == 3 and x_proc.ndim == 1`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several valuable fixes, including correcting function overloads, updating the version, and improving the CI workflow. The lazy initialization for the WeightingFilter to support multichannel audio is a great improvement. However, I've identified a bug in the new lazy initialization logic that can cause a crash when switching from multichannel to single-channel processing. I've provided a detailed comment with a suggested fix for this issue.
| needs_init = self.zi.size == 0 | ||
| if not needs_init and x_proc.ndim > 1: | ||
| needs_init = self.zi.ndim < 3 or self.zi.shape[1] != x_proc.shape[0] |
There was a problem hiding this comment.
The current logic for lazy initialization of the filter state zi does not correctly handle switching from a multi-channel input to a single-channel input. If the filter is first used with a 2D array (multi-channel) and then with a 1D array (single-channel), needs_init will remain False. This will cause sosfilt to be called with a 3D zi and a 1D signal, which raises a ValueError.
The logic to determine if zi needs re-initialization should be updated to cover all cases of shape mismatch between the input signal and the filter state. Here is a suggested replacement that correctly handles all transitions between single-channel and multi-channel inputs:
| needs_init = self.zi.size == 0 | |
| if not needs_init and x_proc.ndim > 1: | |
| needs_init = self.zi.ndim < 3 or self.zi.shape[1] != x_proc.shape[0] | |
| needs_init = self.zi.size == 0 | |
| if not needs_init: | |
| if x_proc.ndim == 1: | |
| # Input is 1D, zi must be 2D | |
| if self.zi.ndim != 2: | |
| needs_init = True | |
| else: # x_proc.ndim > 1 | |
| # Input is 2D, zi must be 3D with matching channel count | |
| if self.zi.ndim != 3 or self.zi.shape[1] != x_proc.shape[0]: | |
| needs_init = True |
CI Results 🚀Test Summary
Technical Benchmark Summary📊 View Benchmark DetailsPyOctaveBand: Technical Benchmark ReportGenerated: 2026-03-08 05:26:08 1. Test Signal Parameters
2. Crossover (Linkwitz-Riley)
3. Precision & Isolation
4. Performance
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_stateful_weighting_filter.py (1)
6-11:⚠️ Potential issue | 🟡 MinorMisplaced docstring.
The docstring is placed after the
from pyoctaveband import WeightingFilterimport statement. Docstrings should be the first statement inside a function, before any other code.📝 Proposed fix
`@pytest.mark.parametrize`("block_size", [8, 256, 1024]) `@pytest.mark.parametrize`("filter_type", ["A", "C", "Z"]) def test_weighting_filter_block_processing_matches_full_signal(block_size: int, filter_type: str): + """ + Ensure that block-wise processing with preserved filter state + produces the same result as filtering the full signal at once. + """ from pyoctaveband import WeightingFilter - """ - Ensure that block-wise processing with preserved filter state - produces the same result as filtering the full signal at once. - """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_stateful_weighting_filter.py` around lines 6 - 11, The function test_weighting_filter_block_processing_matches_full_signal has its docstring placed after the import; move the triple-quoted docstring to be the first statement inside the function (before the line "from pyoctaveband import WeightingFilter") so the docstring is the function's official docstring and the import follows it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pyoctaveband/parametric_filters.py`:
- Around line 106-124: The lazy init misses the case when input goes from
multichannel to single-channel causing a zi shape mismatch; update the init
check around self.zi and x_proc so that if x_proc.ndim == 1 and self.zi.ndim ==
3 (zi has shape (n_sections, n_channels, 2)) you set needs_init = True (or
otherwise reinitialize/reshape zi to 2D) before calling signal.sosfilt;
reference the existing symbols self.zi, x_proc, n_sections, signal.sosfilt_zi
and signal.sosfilt to locate and implement this additional dimension-mismatch
branch.
In `@tests/test_stateful_weighting_filter.py`:
- Around line 58-61: Move the function docstring so it appears immediately after
the def line in test_weighting_filter_multichannel and before the import;
currently the triple-quoted docstring is placed after the local import of
WeightingFilter, so relocate the docstring to directly follow def
test_weighting_filter_multichannel(): to comply with proper docstring placement
for the test and leave the from pyoctaveband import WeightingFilter import
statement below it.
---
Outside diff comments:
In `@tests/test_stateful_weighting_filter.py`:
- Around line 6-11: The function
test_weighting_filter_block_processing_matches_full_signal has its docstring
placed after the import; move the triple-quoted docstring to be the first
statement inside the function (before the line "from pyoctaveband import
WeightingFilter") so the docstring is the function's official docstring and the
import follows it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14937585-337c-448d-9593-bc3ffb0e2b9a
📒 Files selected for processing (5)
.github/workflows/python-app.ymlsrc/pyoctaveband/__init__.pysrc/pyoctaveband/core.pysrc/pyoctaveband/parametric_filters.pytests/test_stateful_weighting_filter.py
| def test_weighting_filter_multichannel(): | ||
| from pyoctaveband import WeightingFilter | ||
| """Stateful block-wise multichannel weighting must match full-signal processing.""" | ||
| rng = np.random.default_rng(99) |
There was a problem hiding this comment.
Same docstring placement issue.
The docstring should be moved before the import statement.
📝 Proposed fix
def test_weighting_filter_multichannel():
+ """Stateful block-wise multichannel weighting must match full-signal processing."""
from pyoctaveband import WeightingFilter
- """Stateful block-wise multichannel weighting must match full-signal processing."""📝 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_weighting_filter_multichannel(): | |
| from pyoctaveband import WeightingFilter | |
| """Stateful block-wise multichannel weighting must match full-signal processing.""" | |
| rng = np.random.default_rng(99) | |
| def test_weighting_filter_multichannel(): | |
| """Stateful block-wise multichannel weighting must match full-signal processing.""" | |
| from pyoctaveband import WeightingFilter | |
| rng = np.random.default_rng(99) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_stateful_weighting_filter.py` around lines 58 - 61, Move the
function docstring so it appears immediately after the def line in
test_weighting_filter_multichannel and before the import; currently the
triple-quoted docstring is placed after the local import of WeightingFilter, so
relocate the docstring to directly follow def
test_weighting_filter_multichannel(): to comply with proper docstring placement
for the test and leave the from pyoctaveband import WeightingFilter import
statement below it.
…age paths - Fix needs_init to handle all ndim transitions (1D↔2D) in WeightingFilter - Add [tool.coverage.run] relative_files=true so coverage.xml uses relative source paths matching sonar.sources=src - Add test_weighting_filter_multichannel_to_mono_transition test
CI Results 🚀Test Summary
Technical Benchmark Summary📊 View Benchmark DetailsPyOctaveBand: Technical Benchmark ReportGenerated: 2026-03-08 05:44:19 1. Test Signal Parameters
2. Crossover (Linkwitz-Riley)
3. Precision & Isolation
4. Performance
|
…coverage - Extract _init_filter_state() and _needs_zi_reinit() helpers from filter() to reduce Cognitive Complexity from 26 to ~5 (SonarCloud max: 15) - Change pip install to editable mode (-e) so coverage can trace src/ code (fixes 'No data was collected' warning and 0% new_coverage in SonarCloud)
jmrplens
force-pushed
the
fix/overloads-and-preexisting-issues
branch
from
1308e22 to
ff2f3f5
Compare
|
CI Results 🚀Test Summary
Technical Benchmark Summary📊 View Benchmark DetailsPyOctaveBand: Technical Benchmark ReportGenerated: 2026-03-08 05:50:12 1. Test Signal Parameters
2. Crossover (Linkwitz-Riley)
3. Precision & Isolation
4. Performance
|
CI Results 🚀Test Summary
Technical Benchmark Summary📊 View Benchmark DetailsPyOctaveBand: Technical Benchmark ReportGenerated: 2026-03-08 05:51:19 1. Test Signal Parameters
2. Crossover (Linkwitz-Riley)
3. Precision & Isolation
4. Performance
|
1 participant
Summary
This PR addresses preexisting issues found after merging PR #42 (stateful block-wise processing).
Fixes
1. Missing
@overloadsignatures inOctaveFilterBank(core.py)calculate_level: Literal[True]overloads so thatmypycorrectly resolves the return type when callingoctave_filter_bank(..., calculate_level=True).sigbands × calculate_levelcombinations.2.
WeightingFiltermultichannelzibug (parametric_filters.py)zi) was initialized with a shape hardcoded for single-channel input(n_sections, 1, 2).scipy.sosfiltrequires(n_sections, n_channels, 2)for multichannel signals.ziinitialization: allocation is deferred to the firstfilter()call where the actual channel count is known, with automatic re-allocation on shape mismatch.3. Version sync (
__init__.py)__version__was still"1.1.2"— updated to"1.1.4"to match the tagged release.4. SonarCloud CI fix (
.github/workflows/python-app.yml)SonarSource/sonarqube-scan-actionfrom@master(unstable) to@v7(stable, recommended by SonarCloud docs).secrets.TOKEN_GHwith standardsecrets.GITHUB_TOKEN(auto-available in all workflows).Tests
test_weighting_filter_steady_ic_initializationfor lazy init pattern.test_weighting_filter_multichannel(4-channel, 2 consecutive blocks).CI checks (local)
ruff✅mypy✅ (0 errors)bandit✅pytest94/94 ✅Summary by Sourcery
Align type hints, stateful filtering behavior, versioning, and CI configuration with expected usage and releases.
Bug Fixes:
Enhancements:
CI:
Chores:
Summary by CodeRabbit
Chores
Improvements
Tests