docs(history): library review — matteospanio/torchfx - #39
Merged
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
slittycode
commented
May 13, 2026
slittycode
left a comment
slittycode
left a comment
Owner
Author
There was a problem hiding this comment.
Verdict: APPROVE
Summary
Docs-only addition of docs/history/library-review-torchfx-2026-05-13.md — a library evaluation ruling out matteospanio/torchfx as a dependency. No code changes, no schema changes, no test impact. The GPL v3 / MIT incompatibility finding is correct and decisive on its own.
Findings
None. The verifiable claims hold:
- Call-site inventory matches the code (
analyze_detection.py:38,97–120,481,554;analyze_core.py:894–903). - ASA is MIT (
/LICENSE). GPL v3 linkage would force the combined work to GPL v3. Recommendation is correct. - Demucs GPU placement claim (
analyze_transcription.py:497, 521) is accurate. docs/history/is the right location per CLAUDE.md ("completed plans and one-shot audits").
Test results
No tests required — docs only.
Phase boundary check
Clean. No code touched.
Generated by Claude Code
slittycode
added a commit
that referenced
this pull request
May 14, 2026
…edBandpass (#44) * docs(history): add torchfx library review 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 * refactor(backend): centralize per-band Butterworth filtering in BatchedBandpass 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 * refactor(backend): accept dtype kwarg on BatchedBandpass.filter_one 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 --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
docs/history/library-review-torchfx-2026-05-13.mdrecording the evaluation ofmatteospanio/torchfxfor ASA.TL;DR
Do not adopt as a dependency. Borrow the pattern only if a future profile shows per-band filtering on the Phase 1 hot path.
Why
LoButterworth,ParametricEQ). Nothing here replaces Essentia's MIR/loudness/key/BPM. The only thing on offer isnn.Moduleplumbing aroundtorchaudio.functional.lfilter, which is ~50 LOC if we ever want it.EXPECTED_TOP_LEVEL_KEYSparity.What's worth borrowing later (if and only if a profile justifies it)
Wave(tensor, fs, device)carrier object — thedevicefield is the actual ergonomic win, enabling co-location with Demucs.nn.Modulefilter wrappers withregister_bufferfor SOS coefficients (no accidental autograd).|for chains — readable in hot loops. Skip+(parallel-mix) — ASA analyzes bands separately, doesn't sum them.apps/backend/dsp_utils.py. Do notpip install torchfx.See the doc for the full call-site inventory in
analyze_detection.py/analyze_core.pyand the borrowed-pattern checklist.https://claude.ai/code/session_01WJ4q6kzeEMscgnSa9gRqQk
Generated by Claude Code