External-repo review + Track 1 verification spike (EBU R128) - #33
Merged
Conversation
…A incorporation Evaluates the three-track incorporation plan against PURPOSE.md and the current ASA baseline (LUFS path in analyze_core.py, JSON schema in JSON_SCHEMA.md, REST surface in server_*.py). Track 1 (LUFS via openmeters): premise unverified. Recommends a 1-day spike (EBU R128 test set + tolerance check) before any rewrite. Flags GPL-3.0 license hazard with openmeters and routes spectral reassignment through librosa.reassigned_spectrogram instead of porting Rust. Track 2 (schema via Partiels): plan misreads Partiels — its CSV is three flat columns, not a richer schema. Reframes to a tabular CSV view per ASA time-series field, modeled on Partiels' (time, duration, value) pattern. Defers SDIF with the paragraph the plan asked for. Track 3 (API via forever-jukebox): side-by-side comparison shows ASA already leads on error envelope, upload size, and staged execution. Adopt list: URL ingestion, public state-machine collapse, audio re-serve, admin key. Reject: Echo Nest output schema, search/discovery, YouTube ingestion. Sequencing matches the plan (2 -> 3 -> 1) with reduced scope: ~6-8 working days total vs the plan's implied effort. https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg
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 PR adding a 306-line evaluation of four external repos against ASA's mission. No code changes. The architectural reasoning is sound and consistent with PURPOSE.md — particularly the rejection of making ASA's JSON "a subset of Partiels" (which would break chain-of-custody invariant #2), the GPL contamination warnings, and the "verify before rewriting" gate on Track 1. Sequencing recommendation is correct.
Findings
Should fix:
docs/external-repo-review-2026-05-13.md, Track 3 side-by-side table — the document says "100 MiB raw / 105 MiB request envelope" for ASA's upload limit. The actual envelope is 101 MiB: MAX_UPLOAD_SIZE_BYTES = 104_857_600 (100 MiB) + UPLOAD_REQUEST_SIZE_SLACK_BYTES = 1_048_576 (1 MiB) = 105,906,176 bytes = 101 MiB. The PR description itself has this right (says 101 MiB). Worth fixing before the doc gets referenced as the canonical source.
Worth considering:
The CSV exporter section calls the proposed …/export/csv/{field_path} route a "JSONPath syntax" path (e.g. lufsCurve.shortTerm). What's described is a simple dot-path, not JSONPath (which uses $. prefix and [*] for arrays). Call it "dot-path" or "field path" to avoid confusion when someone later implements it and reaches for a JSONPath library expecting it to work.
Test results
No tests to run — docs-only PR. No executable code changed.
Phase boundary check
Clean. No code changes; no phase boundary to violate.
Generated by Claude Code
Two corrections to docs/external-repo-review-2026-05-13.md per PR #33 review: 1. Track 3 baseline said '105 MiB request envelope'. Correct value is 101 MiB: MAX_UPLOAD_SIZE_BYTES (104,857,600) + UPLOAD_REQUEST_SIZE_SLACK_BYTES (1,048,576) = 105,906,176 bytes = 101 MiB exactly. The PR description already had this right; the doc body did not. Added the arithmetic inline so the next person doesn't have to redo it. 2. Risks table called the CSV exporter route a 'JSONPath' resolver. What's described is a simple dot-path (e.g. 'lufsCurve.shortTerm'), not JSONPath syntax (which uses '$.' and '[*]'). Clarified to avoid anyone reaching for a JSONPath library when implementing. https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg
… 1 & 2 Adds tests/test_loudness_r128.py that exercises ASA's analyze_loudness path against the EBU R128 Tech 3341 compliance signals: - Case 1: stereo 1 kHz sine at -23 dB FS, 20 s @ 44.1 kHz → expected integrated loudness -23.0 ±0.1 LUFS. - Case 2: stereo 1 kHz sine at -33 dB FS, 20 s @ 44.1 kHz → expected -33.0 ±0.1 LUFS. - Case 1 at 48 kHz with explicit sampleRate=48000 (bypasses analyze_loudness): proves the algorithm itself is sample-rate aware when Essentia is told the right rate. Signals are generated procedurally — no fixture downloads, no audio files committed. This satisfies the review's recommendation in docs/external-repo-review-2026-05-13.md to verify ASA's loudness path against EBU R128 before any port of openmeters' BS.1770-5 implementation. If both 44.1 kHz tests pass on CI, the openmeters port stays rejected and this becomes the regression gate. Open finding surfaced during the spike — filed as a follow-up, NOT fixed here: - analyze_core.analyze_loudness instantiates LoudnessEBUR128() with no sampleRate (defaults to 44100), but load_stereo returns audio at the source file's native rate without resampling. analyze_fast.py already does the right thing (sampleRate=sample_rate). Documented in docs/track1-spike-outcome-2026-05-13.md with a concrete fix sketch and the affected call sites. https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg
slittycode
changed the title
slittycode
added a commit
that referenced
this pull request
May 13, 2026
Closes the open finding from PR #33's Track 1 verification spike. Before this PR, analyze_core.analyze_loudness instantiated es.LoudnessEBUR128() with no sampleRate argument (Essentia default 44100), but the full pipeline at analyze.py:1497 passed in a stereo array from load_stereo at the source file's native rate (no upstream resample). Any source above 44.1 kHz got K-weighting tuned for the wrong rate. This PR: - analyze_loudness now takes sample_rate: int = 44_100 and threads it to LoudnessEBUR128(sampleRate=sample_rate). - analyze.py:1503 (full pipeline) passes sr from load_stereo through. - analyze.py:1242 (stem path) hardcodes 44_100 because Demucs writes stems at 44.1 kHz regardless of source rate. - New regression test asserts -23.0 ±0.1 LUFS through analyze_loudness at 48 kHz. - Three mock-based wiring tests (per review feedback) assert sampleRate=<value> reaches Essentia, including the verbatim case for 96 kHz. - Docstring on the spike outcome doc marks the finding as resolved. https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg
slittycode
added a commit
that referenced
this pull request
May 14, 2026
…nt (#43) * feat(spectral): librosa.reassigned_spectrogram as an opt-in enhancement Last item from the merged external-repo review's adopt list. Adds a new on-demand spectral-enhancement kind, 'reassigned', that produces a sharper spectrogram via per-bin (time, frequency) reassignment. Why reassignment: vanilla STFT spectrograms smear transients across the analyzing window (~46 ms for n_fft=2048 at 44.1 kHz). Reassignment relocates each STFT bin to the local centroid of energy in the (time, freq) plane — transients land at their true onset and stable partials collapse to sharp horizontal lines. Useful when a producer is trying to read attack timing or harmonic detail from the existing mel/STFT views and finding them too blurry. Implementation: - apps/backend/spectral_viz.py — new generate_reassigned_spectrogram function. Calls librosa.reassigned_spectrogram with both frequency and time reassignment enabled, fill_nan=True (handles silent-region numerical instability without dropping points), and renders via matplotlib scatter (not librosa.display.specshow — specshow would re-rasterize to a uniform grid and undo the sharpening). Magnitude floor of -60 dBFS below peak filters the noise wash without losing attack detail. - apps/backend/server.py — registered as 'reassigned' in the existing _ENHANCEMENT_GENERATORS dict. Inherits all existing wiring: auth, measurement-completed precondition, idempotency, artifact storage, MIME types, JSON response envelope. - apps/backend/tests/test_spectral_viz.py — 3 new tests: - produces_single_reassigned_png: shape + non-trivial size - output_is_valid_png: magic bytes - handles_silent_input_without_raising: regression gate against NaN propagation on near-silent fixtures No new dependencies — librosa is already in requirements.txt (librosa==0.11.0 exposes reassigned_spectrogram). Usage: POST /api/analysis-runs/{run_id}/spectral-enhancements/reassigned Returns the new 'spectrogram_reassigned' artifact ID alongside the existing PNG-style spectrograms. Idempotent: re-posting returns the existing artifact without regenerating. Out of scope (deferred): - Frontend UI toggle to display the reassigned PNG. The backend now produces it on demand; the UI wiring is a separate follow-up. - Raw (time, freq, mag) JSON artifact for client-side custom visualization. Could be added as a sibling 'reassigned_data' artifact if a future consumer needs it. Closes the last review-adopt item. Full session summary in the merge commits for #33 through #42. https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg * perf(spectral): cap reassigned-scatter at 300K points PR #43 review caught a real perf issue: the original generate_reassigned_spectrogram fed every STFT cell into matplotlib.scatter unchecked. At default params (n_fft=2048, hop=1024, sr=44100), that's: ~8M raw points on a 3-minute track ~15M raw points on a 10-minute track Both well above the floor-mask survival count and well above the threshold where matplotlib's render step starts taking tens of seconds. The librosa compute cost isn't the bottleneck — the scatter render is. Fix: - New REASSIGNED_MAX_SCATTER_POINTS = 300_000 constant. At FIG_DPI=100 / ~1200x400 px figure, 300K is the visual sweet spot — sharper-looking than vanilla STFT, renders in under a second. - After the floor mask, the post-mask arrays (times_visible, freqs_visible, mags_visible) get subsampled via a seeded RNG (np.random.default_rng(0)). Seeded so re-running on the same audio produces a byte-identical PNG — matters for idempotency tests and content-hash artifact caching. - New test test_scatter_point_cap_is_enforced_for_long_inputs: generates a 60 s multi-component fixture (~1M+ post-mask points, comfortably above the 300K cap), patches matplotlib.scatter to capture call args without rendering, and asserts the actual point count passed to scatter is <= REASSIGNED_MAX_SCATTER_POINTS. The 'inferno colormap' worth-noting from the review was checked — no 'inferno' reference exists in CLAUDE.md or anywhere in the codebase. The reviewer's worth-noting was itself stale; nothing to fix there. https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg --------- 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.
Two layers of work on the external-repo incorporation plan:
docs/external-repo-review-2026-05-13.md) — evaluates openmeters, soundscope, Partiels, and forever-jukebox against PURPOSE.md and ASA's current implementation. Adopt/adapt/reject per track with reasoning.TL;DR — the review
LoudnessEBUR128is likely close enough on stereo material that 1770-5 deltas fall below ASA's reported precision (±0.1 LU). Wrong runtime target (Rust → Python, not Rust → JS — there is no Essentia.js here).time, duration, label/value). ASA's schema is richer, not poorer.X-Admin-Key. Reject: Echo Nest schema, Spotify/YouTube search.Track 1 verification spike — added in commit
8c7da4capps/backend/tests/test_loudness_r128.py— three procedurally-generated tests, no fixture downloads:test_case1_minus23_dbfs_sine_yields_minus23_lufsanalyze_loudness(stereo)test_case2_minus33_dbfs_sine_yields_minus33_lufsanalyze_loudness(stereo)test_case1_at_48khz_with_explicit_sample_ratees.LoudnessEBUR128(sampleRate=48000)directlyThe third probe separates "algorithm correct at non-44.1 rates" from "ASA's call site threads the sample rate" — the difference matters for the open finding below.
docs/track1-spike-outcome-2026-05-13.md— the outcome doc that this spike closes out against. If CI is green, Track 1's openmeters port stays rejected.Open finding: sample-rate threading bug in
analyze_loudnessSurfaced while tracing the call sites for the spike. Not fixed in this PR (out of scope for a verification-only spike), but filed:
analyze_core.py:197—es.LoudnessEBUR128()instantiated with nosampleRate(defaults to 44100).analyze_fast.py:100—es.LoudnessEBUR128(sampleRate=sample_rate)— does the right thing.analyze.py:1448—load_stereoreturns audio at the source's native rate without resampling.So a 48 kHz FLAC reference track today gets its loudness measured against K-weighting filters tuned for 44.1 kHz. The integrated-LUFS error is small at 1 kHz but grows with frequency. Fix sketch (in the outcome doc): thread
sample_ratethroughanalyze_loudness, update call sites atanalyze.py:1242andanalyze.py:1497. Small follow-up PR.What this PR does NOT do
librosa.reassigned_spectrogram— separate small add per the review.Open questions for the human
POST /api/analysis-runsURL-mode lands, do we want host-allowlisting in hosted mode by default?…/export/csv/{field_path}using a simple dot-path (lufsCurve.shortTerm). Open to typed routes per series if preferred.https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg