refactor(backend): centralize per-band Butterworth filtering in BatchedBandpass - #44
Conversation
Library evaluation of matteospanio/torchfx against ASA's per-band filtering call sites. Conclusion: do not adopt — GPL v3 vs ASA's MIT license is a one-way door, the shipped filter inventory is too sparse to displace scipy, and Phase 1 bandpass cost is not on the critical path. The torchfx |/+ operator pattern and the (tensor, fs, device) carrier object are worth borrowing only if a future profile shows per-band filtering is a top-three Phase 1 cost on long tracks. https://claude.ai/code/session_01WJ4q6kzeEMscgnSa9gRqQk
…edBandpass The torchfx audit (docs/history/library-review-torchfx-2026-05-13.md, PR #39) ruled out adopting matteospanio/torchfx as a dependency (GPL v3 vs ASA's MIT) but flagged the underlying pattern — a small filter primitive that batches per-band work and gives a clean hook for future GPU acceleration — as worth borrowing internally. This commit implements the minimum slice of that pattern that earns its complexity today: PR 1 of the audit's plan. Three scipy-Butterworth bandpass call sites in analyze_detection.py (_bandpass_signal, the reverb per-band RT60 loop) were each re-implementing the same butter(4, [lo, hi], output='sos') + sosfiltfilt sequence with slightly different glue. They now share one BatchedBandpass instance per sample rate (cached via functools.lru_cache(maxsize=4)) that memoises SOS coefficients per (lo_hz, hi_hz). The 7-band transient-density loop in analyze_per_band_transient_density and the Essentia BandPass call sites are intentionally out of scope for this PR. Output is bit-identical to the previous inline implementation. The load-bearing test (test_filter_one_matches_inline_scipy_bit_for_bit) compares the new class against a literal copy of the pre-refactor _bandpass_signal arithmetic on a fixed-seed signal and asserts np.allclose(atol=1e-12, rtol=1e-12). The full pre/post snapshot of analyze.py output on a synthetic click-train fixture is zero-diff (including reverbDetail.perBandRt60), so Phase 1 numbers do not move. No JSON field changes, no schema changes, no new dependencies. The BatchedBandpass(backend=...) argument is a forward-looking seam — only "scipy" is implemented today; a future torch path would land here if profiling shows per-band scipy filtering is hot. Tests: - tests.test_dsp_utils.BatchedBandpassTests: 8/8 OK (incl. bit-identical parity for both filter_one and filter_many) - tests.test_analyze.ReverbDetailTests: 7/7 OK (unchanged) - discover -s tests: 551/551 OK https://claude.ai/code/session_01WJ4q6kzeEMscgnSa9gRqQk
slittycode
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
Pure internal refactor of analyze_detection.py's Butterworth bandpass call sites into a new BatchedBandpass utility class. Phase 1 output is bit-identical by construction and verified by a literal copy-paste of the pre-refactor arithmetic as a reference oracle. All 551 backend tests pass per the PR description. No schema changes, no new external dependencies, no JSON field changes.
Findings
Worth considering — filter_one / filter_many output dtype asymmetry (dsp_bandbank.py:67–91)
filter_one hardcodes float32 (it has to — bit-identical parity with _bandpass_signal). filter_many defaults to float64. The two public methods on the same class silently return different dtypes with no-arg calls, which is a footgun for the analyze_per_band_transient_density migration the PR is explicitly anticipating. The current call site (filter_many(..., dtype=np.float32)) is fine, so this is not a regression. But the intended follow-on PR will need to call filter_many with no dtype (float64 result) and then cast only for the librosa call — which is the opposite of what filter_one does. Consider either adding a dtype parameter to filter_one for consistency, or documenting the intentional asymmetry more prominently than a test comment.
This is a judgment call and not a blocker.
Test results
551 / 551 pass (per PR description with command output). BatchedBandpassTests: 8/8, ReverbDetailTests: 7/7. The load-bearing test (test_filter_one_matches_inline_scipy_bit_for_bit) asserts np.allclose(atol=1e-12, rtol=1e-12) against a literal pre-refactor copy — correct approach and the right threshold (well below float32 ULP at this magnitude).
Phase boundary check
Clean. Only analyze_detection.py (Phase 1 internal) and the new dsp_bandbank.py (Phase 1 utility) are modified. No Phase 2/3 files touched. analyze.py stdout contract and JSON_SCHEMA.md are unchanged.
Generated by Claude Code
Addresses review feedback on PR #44: the original filter_one hardcoded float32 while filter_many defaulted to float64, which would be a footgun for the planned analyze_per_band_transient_density migration (PR 2 of the audit). filter_one now accepts a keyword-only ``dtype`` argument, making the public API symmetric with filter_many. The default remains ``np.float32`` — bit-identicality with the pre-refactor _bandpass_signal is preserved, and the existing reverb call site is unchanged. The asymmetric defaults (float32 for filter_one, float64 for filter_many) are intentional and now documented in a class-level note: each default matches its primary use case verbatim, and callers can override either at the call site. Two new tests lock the contract: - test_filter_one_default_dtype_is_float32 — asserts default is float32 (bit-identicality guarantee with _bandpass_signal) - test_filter_one_respects_dtype_override — asserts dtype=np.float64 produces a float64 array (the forward-looking override path) BatchedBandpassTests: 10/10 OK. ReverbDetailTests: 7/7 OK unchanged. https://claude.ai/code/session_01WJ4q6kzeEMscgnSa9gRqQk
Context
The torchfx audit (
docs/history/library-review-torchfx-2026-05-13.md, PR #39) ruled out adoptingmatteospanio/torchfxas a dependency — GPL v3 vs ASA's MIT is a one-way door — but flagged the underlying pattern as worth borrowing: a small filter primitive that batches per-band work and gives a clean hook for future GPU acceleration. This PR implements the minimum slice of that pattern that earns its complexity today: PR 1 of the audit's plan.What changed
Three scipy-Butterworth bandpass call sites in
analyze_detection.py(_bandpass_signal, the reverb per-band RT60 loop) were each re-implementing the samebutter(4, [lo, hi], output='sos') + sosfiltfiltsequence with slightly different glue. They now share oneBatchedBandpassinstance per sample rate (cached viafunctools.lru_cache(maxsize=4)) that memoises SOS coefficients per(lo_hz, hi_hz).The 7-band transient-density loop in
analyze_per_band_transient_densityand the EssentiaBandPasscall sites are intentionally out of scope for this PR. The transient-density swap is a candidate for a follow-on PR if and only if this one lands cleanly.Files
apps/backend/dsp_bandbank.pyBatchedBandpassclass (~120 LOC incl. docstrings)apps/backend/analyze_detection.py_bandpass_signalshrunk to one-line delegation; reverb loop dispatches viabandbank.filter_many(_REVERB_BANDS, dtype=np.float32)apps/backend/tests/test_dsp_utils.pyBatchedBandpassTestsclass (8 tests)Bit-identicality
Output is bit-identical to the previous inline implementation. The load-bearing test
test_filter_one_matches_inline_scipy_bit_for_bitcompares the new class against a literal copy of the pre-refactor_bandpass_signalarithmetic on a fixed-seed signal and assertsnp.allclose(atol=1e-12, rtol=1e-12). The full pre/post snapshot ofanalyze.pyoutput on a synthetic click-train fixture is zero-diff — includingreverbDetail.perBandRt60. Phase 1 numbers do not move.No JSON field changes, no schema changes, no new dependencies.
Forward-looking seam
BatchedBandpass(backend=...)is reserved for a future torch path — only"scipy"is implemented today; passing anything else raisesValueError. A future torch backend would land here if profiling shows per-band scipy filtering is hot. Until then, this is the simplest shape that earns its keep on consolidation alone.Tests
End-to-end pre/post snapshot of
analyze.pyon/tmp/reverb_test.wav(synthetic click train + 80 Hz drone):diffproduces zero output.Why this is worth it
This is a maintenance change per
PURPOSE.mddecision framework category #4 — it doesn't move Phase 1 numbers, doesn't change Phase 2 recommendations, isn't on the critical latency path. What it does buy: one place to add a torch-backedbackend="torch"variant if profiling ever shows per-band scipy filtering is a hotspot, plus the audit conclusion ("don't add the dep, borrow the pattern") now has a concrete shape in the tree.https://claude.ai/code/session_01WJ4q6kzeEMscgnSa9gRqQk
Generated by Claude Code