From 705d07461d7f2a63118cc80f583d316a16d7eee5 Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Tue, 8 Sep 2026 16:29:31 +0200 Subject: [PATCH 1/3] Refuse an explicit window_s a clip cannot hold (#43) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `align_clips_to_reference(..., window_s=45)` on a 50 s clip was accepted, produced one window, and returned `support=None`. The caller asked for a windowed vote and got a single whole-clip correlation; the only signal was the ABSENCE of a number, which is indistinguishable from a clip too short to support at any window. Above `clip_duration - hop_s` the clip holds one window, so the vote cannot be held. That is now `WindowTooWideForClip` — a new typed error carrying `clip_index`, `clip_duration_s`, `window_s`, `hop_s` and `max_window_s` (the largest window that still leaves a second, independent look), subclassing `ValueError` so existing handlers around alignment keep working. `mixing/errors.py` is a new pure module, the home for typed errors, reachable through the lazy facade without pulling a backend. BREAKING: calls passing an explicit `window_s` wider than the clip now raise where they returned an uncorroborated answer. Only the EXPLICIT path is refused — with `window_s=None` the window is still fitted per clip and every clip is answered, including one that lands on a single window at the floor. Six tests that pinned the silent behaviour now pin the refusal; a new test pins the default path unchanged. Claude-Session: https://claude.ai/code/session_01XY8uj3B4XKHKCqbfBRxgeX --- .claude/skills/mixing-audio/SKILL.md | 2 + mixing/__init__.py | 5 + mixing/audio/__init__.py | 3 + mixing/audio/audio_ops.py | 44 +++++++- mixing/errors.py | 81 ++++++++++++++ mixing/tests/test_adaptive_window.py | 97 ++++++++++++++--- mixing/tests/test_aligned_spans.py | 154 ++++++++++++++++++--------- 7 files changed, 315 insertions(+), 71 deletions(-) create mode 100644 mixing/errors.py diff --git a/.claude/skills/mixing-audio/SKILL.md b/.claude/skills/mixing-audio/SKILL.md index 498b50a..434bb98 100644 --- a/.claude/skills/mixing-audio/SKILL.md +++ b/.claude/skills/mixing-audio/SKILL.md @@ -240,6 +240,8 @@ that spread but does not remove it. Scale your threshold to `window_s`, or pass `window_s` to put every clip on one scale. `aligned_spans` is NOT adapted — there `window_s` is boundary resolution, which is the caller's to choose. +**An explicit `window_s` wider than `clip_duration - hop_s` raises `WindowTooWideForClip`** (0.0.53, issue #43). Above that the clip holds a single window, so the vote you asked for cannot be held: before, you got one whole-clip correlation and a `support=None` that looked exactly like a clip too short to support at any window. The error is importable from `mixing` or `mixing.errors`, subclasses `ValueError`, and carries `clip_index`, `clip_duration_s`, `window_s`, `hop_s` and `max_window_s` — the largest window that still leaves a second look, so a caller putting a mixed-length set on one scale can retry at a window every clip can hold. `max_window_s <= 0` means the clip is shorter than the hop and no window works. This is the explicit path only: `window_s=None` still fits the window to each clip and answers every clip, including one that lands on a single window at the floor. + **Reading `window_s` is necessary and, since 0.0.49, no longer sufficient.** A rule of the form *"below the default window, treat the value as unmeasured"* was a complete defence while `support` was an argmax headcount, because the window was the only axis that moved diff --git a/mixing/__init__.py b/mixing/__init__.py index aae87bb..8e38f8d 100644 --- a/mixing/__init__.py +++ b/mixing/__init__.py @@ -103,6 +103,9 @@ "dub_video_from_srt": "mixing.dubbing", "text_to_speech": "mixing.dubbing", "translate_srt": "mixing.dubbing", + # typed errors (pure) + "MixingError": "mixing.errors", + "WindowTooWideForClip": "mixing.errors", # srt / timeline (pure) "Cue": "mixing.srt", "parse_srt": "mixing.srt", @@ -120,6 +123,7 @@ "chapters", "srt", "egress", + "errors", "util", ) @@ -177,6 +181,7 @@ transcribe, ) from mixing.dubbing import dub_video_from_srt, text_to_speech, translate_srt # noqa: F401 + from mixing.errors import MixingError, WindowTooWideForClip # noqa: F401 def __getattr__(name: str): diff --git a/mixing/audio/__init__.py b/mixing/audio/__init__.py index 822b186..3141e72 100644 --- a/mixing/audio/__init__.py +++ b/mixing/audio/__init__.py @@ -65,6 +65,7 @@ segment_by_speech_music, ) from .beats import beat_grid, BeatGrid +from ..errors import MixingError, WindowTooWideForClip __all__ = [ "Audio", @@ -93,6 +94,8 @@ "ClipAlignment", "onset_envelope", "ALIGNMENT_FEATURES", + "MixingError", + "WindowTooWideForClip", "Segment", "find_segments", "extract_segments", diff --git a/mixing/audio/audio_ops.py b/mixing/audio/audio_ops.py index 3e50fa3..0c62439 100644 --- a/mixing/audio/audio_ops.py +++ b/mixing/audio/audio_ops.py @@ -50,6 +50,7 @@ from ..util import require_package, AudioTimeUnit, to_seconds, get_path_from_clipboard from .audio_util import AudioSource, _normalize_audio_source from ..egress import Output, deliver, is_sink, resolve_output_path +from ..errors import WindowTooWideForClip logger = logging.getLogger(__name__) @@ -2133,6 +2134,7 @@ def _clip_window_and_hop( *, window_s: "float | None", hop_s: "float | None", + clip_index: "int | None" = None, default_window_s: float = SPAN_WINDOW_S, target_windows: int = MIN_WINDOWS_FOR_SUPPORT_TARGET, min_frames: int = ADAPTIVE_WINDOW_MIN_FRAMES, @@ -2173,12 +2175,40 @@ def _clip_window_and_hop( real alignments read 0.45/0.64/0.73 at 20 s and 0.19/0.16/0.23 at 5 s. A short clip had no support at all before, so nothing is being reinterpreted; but a caller gating on support must know that the number arrives on the clip's scale, not the default's. + + **An EXPLICIT window too wide for the clip is refused, not honored** (issue #43). + Above ``clip_duration_s - hop_s`` the clip holds one window, so the vote the caller + asked for cannot be held and ``support`` comes back ``None`` — which reads exactly + like a clip too short to support at any window. Rather than degrade silently, this + raises :class:`~mixing.errors.WindowTooWideForClip`, naming the clip, its duration + and the largest window that still leaves a second look. Only an explicit window is + refused: the ``None`` path above fits the window to the clip and, at the floor, may + legitimately land on a single window, which is the honest answer for a clip that + short and not a caller's mistake. """ + explicit_window = window_s is not None if window_s is None: floor_s = min_frames * envelope_hop / sample_rate window_s = min(default_window_s, max(floor_s, clip_duration_s / target_windows)) + explicit_hop = hop_s is not None if hop_s is None: hop_s = window_s * hop_ratio + if explicit_window and window_s > clip_duration_s - hop_s: + # The hop is what a second window costs, so the largest window that still + # leaves room for one is `duration - hop` when the hop is the caller's, and + # `duration / (1 + ratio)` when the hop rides on the window. + max_window_s = ( + clip_duration_s - hop_s + if explicit_hop + else clip_duration_s / (1.0 + hop_ratio) + ) + raise WindowTooWideForClip( + clip_index=clip_index, + clip_duration_s=clip_duration_s, + window_s=window_s, + hop_s=hop_s, + max_window_s=max_window_s, + ) return float(window_s), float(hop_s) @@ -2259,8 +2289,17 @@ def align_clips_to_reference( the scale its ``support`` is on. A clip of three default windows or more is measured at :data:`SPAN_WINDOW_S` exactly, so nothing about a long clip's answer moves. Pass a number to fix the window yourself — an explicit - value is never overridden, and it is how the pre-adaptation answer for a short - clip is reproduced. + value is never overridden. + + **An explicit window wider than ``clip_duration - hop_s`` is REFUSED** with + :class:`~mixing.errors.WindowTooWideForClip` (issue #43). Above that length + the clip holds a single window, so the vote asked for cannot be held and the + result would be one whole-clip correlation reporting ``support=None`` — the + same thing a clip too short to support at any window reports, with nothing to + tell the two apart. The error names the clip, its duration and the largest + window that still leaves a second look. To measure such a clip, pass that + window, pass ``window_s=None`` to fit the window to each clip, or pass + ``consensus=False`` to ask for the single correlation outright. hop_s: Step between those windows. Ignored when ``consensus`` is False. ``None`` (the default) is :data:`ADAPTIVE_HOP_RATIO` of whatever window is in force — half of it, the default pair's own ratio — so an adapted grid keeps the @@ -2306,6 +2345,7 @@ def align_clips_to_reference( sample_rate, window_s=window_s, hop_s=hop_s, + clip_index=i, ) offset_s, coeff, support, margin = _consensus_alignment( ref, diff --git a/mixing/errors.py b/mixing/errors.py new file mode 100644 index 0000000..81ce848 --- /dev/null +++ b/mixing/errors.py @@ -0,0 +1,81 @@ +"""Typed errors `mixing` raises — the home for exceptions a caller may want to catch. + +Pure stdlib, imported by the lazy facade without pulling any backend, so a consumer +can ``from mixing import WindowTooWideForClip`` in a light module and still catch what +a heavy one raises. + +The convention: an error class carries the MEASURED numbers as attributes, not only in +its message, so a caller can act on them (retry at the largest valid window, say) rather +than parse prose. +""" + +__all__ = ["MixingError", "WindowTooWideForClip"] + + +class MixingError(Exception): + """Base for every error `mixing` raises on its own behalf.""" + + +class WindowTooWideForClip(MixingError, ValueError): + """An explicit analysis window leaves a clip no second, independent look. + + Raised by :func:`mixing.audio.align_clips_to_reference` when the caller passes a + ``window_s`` wider than ``clip_duration_s - hop_s`` — the length below which the + clip holds only one window, so the consensus vote has nothing to arbitrate and + ``support`` comes back ``None``. + + Why an error and not a clamp or a silent single window: the caller asked for a + windowed vote and would otherwise receive a single whole-clip correlation whose + only signal is the ABSENCE of a support number — indistinguishable from a clip + that is genuinely too short to support at any window (issue #43). Clamping would + be worse still: an explicit ``window_s`` is the caller saying what a window means + for their material, and silently measuring at a different one makes ``support`` + incomparable across a set for a reason nothing reports. + + It subclasses :class:`ValueError` because it is an argument that cannot be honored, + so existing ``except ValueError`` handlers around alignment keep working. + + Attributes: + clip_index: Position of the offending clip in the ``clips`` sequence, or + ``None`` when the caller did not identify one. + clip_duration_s: The clip's duration in seconds. + window_s: The window that was asked for. + hop_s: The hop in force — the caller's, or the one derived from ``window_s``. + max_window_s: The largest window that would still leave this clip a second + look, given ``hop_s``. ``<= 0`` means no window can: the clip is shorter + than the hop, and only ``consensus=False`` (or a smaller ``hop_s``) + applies. + """ + + def __init__( + self, + *, + clip_index: "int | None", + clip_duration_s: float, + window_s: float, + hop_s: float, + max_window_s: float, + ) -> None: + self.clip_index = clip_index + self.clip_duration_s = float(clip_duration_s) + self.window_s = float(window_s) + self.hop_s = float(hop_s) + self.max_window_s = float(max_window_s) + who = "clip" if clip_index is None else f"clip {clip_index}" + if self.max_window_s > 0: + remedy = ( + f"pass window_s <= {self.max_window_s:.3f}, or window_s=None to fit " + f"the window to each clip" + ) + else: + remedy = ( + f"no window holds a second look at hop_s={self.hop_s:.3f}; pass a " + f"smaller hop_s, window_s=None, or consensus=False" + ) + super().__init__( + f"window_s={self.window_s:.3f} is wider than {who} " + f"({self.clip_duration_s:.3f} s) allows at hop_s={self.hop_s:.3f}: the clip " + f"would hold a single window, so the consensus vote has nothing to " + f"arbitrate and support would be None for a reason nothing reports — " + f"{remedy}." + ) diff --git a/mixing/tests/test_adaptive_window.py b/mixing/tests/test_adaptive_window.py index 77ecbf7..a2c349f 100644 --- a/mixing/tests/test_adaptive_window.py +++ b/mixing/tests/test_adaptive_window.py @@ -10,6 +10,12 @@ clip gets a real vote and a measured support, and a clip long enough to hold the default window is measured at the default window *exactly* — same offset, same confidence, same support, to the bit. + +Issue #43 closed the other side of the same door. Fitting the window is what the DEFAULT +does; an EXPLICIT window is still never overridden, but one the clip cannot hold is now +refused (:class:`~mixing.errors.WindowTooWideForClip`) instead of quietly delivering the +single-window answer this module exists to have replaced. Several tests below pin that +boundary, and the calls they make are the ones that used to return ``support=None``. """ import numpy as np @@ -17,6 +23,7 @@ from scipy.io import wavfile from mixing.audio import align_clips_to_reference +from mixing.errors import WindowTooWideForClip from mixing.audio.audio_ops import ( ADAPTIVE_HOP_RATIO, ADAPTIVE_WINDOW_MIN_FRAMES, @@ -157,14 +164,58 @@ def test_the_floor_follows_the_analysis_rate_not_the_clock(): def test_an_explicit_window_is_never_overridden(): - """The caller has said what a window means for their material; the rule has not.""" - assert _clip_window_and_hop(5.0, SR, window_s=20.0, hop_s=10.0) == (20.0, 10.0) - assert _clip_window_and_hop(5.0, SR, window_s=20.0, hop_s=None) == ( + """The caller has said what a window means for their material; the rule has not. + + Stated on a clip the window FITS. It used to be stated on a 5 s clip, where the + window could not be honored at all — the rule left it alone there too, and the + result was a single-window answer with ``support=None`` and nothing saying the + window was the reason. That case is now a refusal (issue #43, below); what survives + unchanged is the part that was ever meaningful: where the window can be honored, it + is, exactly as given. + """ + assert _clip_window_and_hop(60.0, SR, window_s=20.0, hop_s=10.0) == (20.0, 10.0) + assert _clip_window_and_hop(60.0, SR, window_s=20.0, hop_s=None) == ( 20.0, 20.0 * ADAPTIVE_HOP_RATIO, ) +def test_an_explicit_window_the_clip_cannot_hold_is_refused(): + """Issue #43: the boundary of "never overridden" is a refusal, not a silent fallback. + + Above ``clip_duration - hop_s`` the clip holds one window, so the vote the caller + named cannot be held. Neither honoring it (one correlation, ``support=None``, no + reason given) nor clamping it (a support on a scale nobody asked for) is reportable, + so the call refuses and the error carries the largest window that would work. + + The bound is exact and inclusive: at ``window_s == clip_duration - hop_s`` a second + window still starts a hop in, so it is accepted. + """ + assert _clip_window_and_hop(30.0, SR, window_s=20.0, hop_s=10.0) == (20.0, 10.0) + + with pytest.raises(WindowTooWideForClip) as excinfo: + _clip_window_and_hop(29.0, SR, window_s=20.0, hop_s=10.0, clip_index=3) + err = excinfo.value + assert (err.clip_index, err.window_s, err.hop_s) == (3, 20.0, 10.0) + assert err.clip_duration_s == 29.0 + assert err.max_window_s == pytest.approx(19.0), "duration - the caller's hop" + assert "clip 3" in str(err) + + # With the hop riding on the window, a wider window buys its own wider hop, so the + # ceiling is duration / (1 + ratio) rather than duration - hop. + with pytest.raises(WindowTooWideForClip) as excinfo: + _clip_window_and_hop(24.0, SR, window_s=20.0, hop_s=None) + assert excinfo.value.max_window_s == pytest.approx(24.0 / (1 + ADAPTIVE_HOP_RATIO)) + assert excinfo.value.clip_index is None and "clip (" in str(excinfo.value) + + # Shorter than the hop itself: no window at all can hold a second look, and the + # message must say that rather than name an impossible ceiling. + with pytest.raises(WindowTooWideForClip) as excinfo: + _clip_window_and_hop(8.0, SR, window_s=20.0, hop_s=10.0) + assert excinfo.value.max_window_s <= 0 + assert "consensus=False" in str(excinfo.value) + + #: The smallest clip that can hold two half-window-separated looks at the floor window: #: one window, then a second starting a hop later. Below it no windowing can produce a #: quorum and ``support=None`` is the true answer, not a miss. @@ -253,8 +304,9 @@ def test_a_short_clip_now_gets_a_vote_and_a_measured_support( At the default the clip is measured at a window fitted to it, so several independent windows look at it and agree. The comparison is with the same call at the default - *pair* — which is what this clip used to get — where there is exactly one window, - nothing to arbitrate, and ``support`` is honestly ``None``. + *pair* — which is what this clip used to get. That call returned one window and an + honest ``support=None``; since issue #43 it is refused outright, which is the same + statement made where the caller can see it: this clip cannot be measured at 20 s. """ reference_path, reference = repeating_reference clip = _excerpt( @@ -262,11 +314,15 @@ def test_a_short_clip_now_gets_a_vote_and_a_measured_support( ) (fitted,) = align_clips_to_reference(reference_path, [clip], sample_rate=SR) - (one_window,) = align_clips_to_reference( - reference_path, [clip], sample_rate=SR, window_s=SPAN_WINDOW_S, hop_s=SPAN_HOP_S - ) + with pytest.raises(WindowTooWideForClip): + align_clips_to_reference( + reference_path, + [clip], + sample_rate=SR, + window_s=SPAN_WINDOW_S, + hop_s=SPAN_HOP_S, + ) - assert one_window.support is None, "the fixture must start out unmeasured" assert fitted.offset_s == pytest.approx(TRUE_OFFSET_S, abs=0.05) assert fitted.support is not None assert 0.0 < fitted.support <= 1.0 @@ -289,11 +345,18 @@ def test_a_clip_just_LONGER_than_one_default_window_is_rescued_too( clip = _excerpt(tmp_path, reference, "just_over.wav", TRUE_OFFSET_S, 22.0, seed=29) (fitted,) = align_clips_to_reference(reference_path, [clip], sample_rate=SR) - (default_grid,) = align_clips_to_reference( - reference_path, [clip], sample_rate=SR, window_s=SPAN_WINDOW_S, hop_s=SPAN_HOP_S - ) + # Longer than a window, still only one INDEPENDENT look — and since issue #43 that + # is refused rather than answered with an unexplained ``support=None``. + with pytest.raises(WindowTooWideForClip) as excinfo: + align_clips_to_reference( + reference_path, + [clip], + sample_rate=SR, + window_s=SPAN_WINDOW_S, + hop_s=SPAN_HOP_S, + ) + assert excinfo.value.max_window_s < SPAN_WINDOW_S - assert default_grid.support is None, "longer than a window, still only one look" assert fitted.offset_s == pytest.approx(TRUE_OFFSET_S, abs=0.05) assert fitted.support is not None assert 0.0 < fitted.support <= 1.0 @@ -340,7 +403,9 @@ def test_the_window_support_is_relative_to_comes_back_with_it( short = _excerpt( tmp_path, reference, "scale_short.wav", TRUE_OFFSET_S, SHORT_CLIP_S, seed=31 ) - long_clip = _excerpt(tmp_path, reference, "scale_long.wav", 5.0, LONG_CLIP_S, seed=33) + long_clip = _excerpt( + tmp_path, reference, "scale_long.wav", 5.0, LONG_CLIP_S, seed=33 + ) short_result, long_result = align_clips_to_reference( reference_path, [short, long_clip], sample_rate=SR @@ -357,8 +422,8 @@ def test_the_window_support_is_relative_to_comes_back_with_it( def test_the_reported_window_is_the_one_asked_for_when_one_is_asked_for(): """An explicit window is reported unchanged, so the field never lies about the scale.""" - reference = np.zeros(int(90 * SR)) - clip = np.zeros(int(30 * SR)) + reference = np.zeros(int(300 * SR)) + clip = np.zeros(int(120 * SR)) (pinned,) = align_clips_to_reference( reference, [clip], sample_rate=SR, window_s=SPAN_WINDOW_S, hop_s=SPAN_HOP_S ) diff --git a/mixing/tests/test_aligned_spans.py b/mixing/tests/test_aligned_spans.py index 0168b14..21396a7 100644 --- a/mixing/tests/test_aligned_spans.py +++ b/mixing/tests/test_aligned_spans.py @@ -23,9 +23,11 @@ aligned_spans, find_audio_offset_detailed, ) +from mixing.errors import WindowTooWideForClip SR = 16000 WIN, HOP = 10.0, 5.0 +SPAN_WINDOW_S_DEFAULT = 20.0 def _reference(seconds: float = 90.0) -> np.ndarray: @@ -666,41 +668,44 @@ def test_support_localises_a_clip_that_is_only_partly_the_song(song, ref, tmp_pa assert 0.3 < a.support < 0.8, f"about half the clip should agree, got {a.support}" -def test_a_clip_shorter_than_one_window_takes_the_whole_clip_answer( - song, ref, tmp_path -): - """Consensus must degrade to the thing it replaces, not to a different thing. - - A clip shorter than ``window_s`` is a single window, so there is nothing to vote on - and the answer — offset, confidence and support — must equal the single-correlation - path exactly. - - ``window_s`` is passed explicitly here, and that is the whole change issue #41 made: - a 15 s clip at the DEFAULT window is no longer one window, because the default now - fits the window to the clip (:func:`_clip_window_and_hop`) precisely so that a short - clip gets the vote this test describes the absence of. The statement itself is - unchanged and still worth pinning — whenever a clip really is one window, however - that came about, consensus must return the single correlation's answer and not a - different one. - - Both sides report ``support=None``: one window is not a quorum, and the equality - would be satisfied just as well by both sides manufacturing 1.0, so that is asserted - separately below rather than left to the tuple comparison. +def test_an_explicit_window_wider_than_the_clip_is_refused(song, ref, tmp_path): + """A window the clip cannot hold is an error, not a silently ungoverned answer. + + This test used to assert the opposite: that consensus at ``window_s=SPAN_WINDOW_S`` + on a 15 s clip degraded to the single whole-clip correlation, same offset, same + confidence, ``support=None``. It degraded correctly — and unreportably. The caller + asked for a windowed vote, got one correlation, and the only trace was the ABSENCE + of a support number, which is exactly what a clip too short to support at any window + also shows (issue #43). So the explicit-window path now refuses. + + What is NOT refused, and is pinned separately below: the ``window_s=None`` path, + which fits the window to the clip and may honestly land on a single window at the + floor. That is the estimator doing its best for a short clip, not a caller's mistake. """ from mixing.audio.audio_ops import SPAN_HOP_S, SPAN_WINDOW_S rng = np.random.default_rng(37) clip = _write(tmp_path, "short_clip.wav", _take(ref, 30, 45, rng)) - (new,) = align_clips_to_reference( - song, [clip], sample_rate=SR, window_s=SPAN_WINDOW_S, hop_s=SPAN_HOP_S - ) - (old,) = align_clips_to_reference(song, [clip], sample_rate=SR, consensus=False) - assert (new.offset_s, new.confidence, new.support) == ( - old.offset_s, - old.confidence, - old.support, + with pytest.raises(WindowTooWideForClip) as excinfo: + align_clips_to_reference( + song, [clip], sample_rate=SR, window_s=SPAN_WINDOW_S, hop_s=SPAN_HOP_S + ) + err = excinfo.value + assert err.clip_index == 0 + assert err.window_s == SPAN_WINDOW_S and err.hop_s == SPAN_HOP_S + assert err.clip_duration_s == pytest.approx(15.0, abs=0.05) + # duration - hop: the largest window that still leaves room for a second look. + assert err.max_window_s == pytest.approx(5.0, abs=0.05) + # The numbers must be IN the message too — the point of the error is that the + # window, not the clip's length alone, is nameable as the reason. + text = str(err) + assert "window_s=20.000" in text and "clip 0" in text + assert isinstance(err, ValueError), "existing ValueError handlers keep working" + # And the window the error names is one the call accepts. + (ok,) = align_clips_to_reference( + song, [clip], sample_rate=SR, window_s=err.max_window_s, hop_s=SPAN_HOP_S ) - assert new.support is None, "one window is not a quorum" + assert ok.window_s == pytest.approx(err.max_window_s) # -------------------------------------------------------------------------- @@ -712,21 +717,20 @@ def test_a_clip_shorter_than_one_window_takes_the_whole_clip_answer( # -------------------------------------------------------------------------- -def test_support_is_None_where_no_second_opinion_exists(two_halves, tmp_path): - """The reviewer's reproduction, and the reason ``None`` is not spelled ``1.0``. +def test_the_one_window_reproduction_is_now_a_refusal(two_halves, tmp_path): + """The reviewer's reproduction, converted from a silent ``None`` to a refusal. A 15 s clip taken from offset 30.0 of a reference that is one 45 s half twice comes - back at offset **75.0** — wrong — at confidence 0.979, on BOTH paths, because it is - a single window and there is nothing for a vote to do. That the answer is wrong is - not this test's complaint; a single window genuinely cannot tell those two halves - apart. The complaint is what ``support`` says about it. Reporting 1.0 (as the first - version of this fix did) turns "nobody checked" into "everybody agreed", and a - consumer gating on ``support >= 1.0`` then waves the wrong offset straight through. - - ``window_s`` is pinned to the shipped default rather than left to it: since issue #41 - the default fits the window to the clip, so this 15 s clip is no longer one window - unless it is asked to be. What that adaptation does to this very fixture is the - subject of the test below. + back at offset **75.0** — wrong — at confidence 0.979 when it is measured as a single + window, because a single window genuinely cannot tell those two halves apart. The + original complaint was about what ``support`` said of that: reporting 1.0 turns + "nobody checked" into "everybody agreed", so it reports ``None``. + + ``None`` was the right answer for the vote and the wrong answer for the CALL: asking + for a 20 s window on a 15 s clip cannot produce the vote it names, and issue #43 says + so out loud rather than handing back an uncorroborated 75.0. ``consensus=False`` still + returns that same wrong offset with ``support=None`` — asked for explicitly, it is an + answer and not a degradation, and it stays pinned here as the contrast. """ from mixing.audio.audio_ops import SPAN_HOP_S, SPAN_WINDOW_S @@ -734,16 +738,48 @@ def test_support_is_None_where_no_second_opinion_exists(two_halves, tmp_path): rng = np.random.default_rng(38) clip = _write(tmp_path, "one_window.wav", _take(ref, 30, 45, rng)) # true offset 30 - (one_window,) = align_clips_to_reference( - song, [clip], sample_rate=SR, window_s=SPAN_WINDOW_S, hop_s=SPAN_HOP_S - ) + with pytest.raises(WindowTooWideForClip) as excinfo: + align_clips_to_reference( + song, [clip], sample_rate=SR, window_s=SPAN_WINDOW_S, hop_s=SPAN_HOP_S + ) + assert excinfo.value.max_window_s == pytest.approx(5.0, abs=0.05) + (single,) = align_clips_to_reference(song, [clip], sample_rate=SR, consensus=False) - assert one_window.offset_s == 75.0 and single.offset_s == 75.0, ( + assert single.offset_s == 75.0, ( "the fixture is only meaningful while the offset really is wrong" ) - assert one_window.confidence > 0.9 - assert one_window.support is None, "one window is not a quorum" - assert single.support is None, "and consensus=False put nothing to a vote at all" + assert single.confidence > 0.9 + assert single.support is None, "consensus=False put nothing to a vote at all" + + +def test_the_default_window_path_is_untouched_by_the_refusal(two_halves, ref, tmp_path): + """The refusal is about EXPLICIT windows only — ``window_s=None`` is byte-identical. + + Issue #43 converts a working call into an error, so the boundary of that change is + worth pinning rather than inferring: with no explicit ``window_s`` the window is + fitted per clip, every clip from long to floor-short is accepted, and the numbers + that come back are the ones the adaptive path produced before the refusal existed. + + The floor case is the one that would break first if the check ever leaked into the + ``None`` path: a clip short enough that the fitted window is the floor may hold a + single window, which is precisely the shape the explicit path now refuses — and here + it must still be answered, because nobody asked for a window the clip cannot hold. + """ + song, _ = two_halves + rng = np.random.default_rng(39) + clips = [ + _write(tmp_path, "long.wav", _take(ref, 5, 65, rng)), # 60 s, default window + _write(tmp_path, "mid.wav", _take(ref, 30, 45, rng)), # 15 s, fitted window + _write(tmp_path, "tiny.wav", _take(ref, 10, 12, rng)), # 2 s, floor window + ] + got = align_clips_to_reference(song, clips, sample_rate=SR) + assert [a.index for a in got] == [0, 1, 2], "no clip was refused" + assert got[0].window_s == pytest.approx(SPAN_WINDOW_S_DEFAULT) + assert got[1].window_s == pytest.approx(5.0, abs=0.05), "min(20, 15/3)" + assert got[2].window_s < 5.0, "the floor window, fitted to a 2 s clip" + assert all(a.hop_s == pytest.approx(a.window_s / 2) for a in got) + # A single-window answer is still an ANSWER on this path, not an error. + assert got[2].offset_s is not None and got[2].confidence is not None def test_the_same_clip_gets_a_measured_support_once_its_window_fits_it( @@ -776,7 +812,9 @@ def test_the_same_clip_gets_a_measured_support_once_its_window_fits_it( (fitted,) = align_clips_to_reference(song, [clip], sample_rate=SR) assert fitted.offset_s == pytest.approx(30.0, abs=0.05) - assert fitted.support is not None, "a fitted window is a vote, and a vote is measured" + assert fitted.support is not None, ( + "a fitted window is a vote, and a vote is measured" + ) assert 0.0 < fitted.support <= 1.0 @@ -812,7 +850,9 @@ def test_windows_that_are_the_same_look_twice_do_not_count_as_support( (crowded,) = align_clips_to_reference( song, [clip], sample_rate=SR, window_s=9.5, hop_s=0.5 ) - assert crowded.offset_s == pytest.approx(20.0, abs=0.05), "the offset still measures" + assert crowded.offset_s == pytest.approx(20.0, abs=0.05), ( + "the offset still measures" + ) assert crowded.support is None, "but 95%-overlapping windows are not two opinions" (spaced,) = align_clips_to_reference( @@ -842,9 +882,17 @@ def _spaced_windows(starts, length: float): [ ([0.0, 10.0, 20.0, 30.0], 4, "a hop of exactly half a window keeps them all"), ([0.0, 10.1, 20.2, 30.3], 4, "and anything wider certainly does"), - ([0.0, 5.0, 10.0, 15.0, 20.0], 3, "a quarter-window hop counts every other one"), + ( + [0.0, 5.0, 10.0, 15.0, 20.0], + 3, + "a quarter-window hop counts every other one", + ), ([0.0, 0.5], 1, "95% overlap is one look, not two — the real-material case"), - ([0.0, 10.0, 20.0, 30.0, 35.0], 4, "the appended TAIL window is not a new look"), + ( + [0.0, 10.0, 20.0, 30.0, 35.0], + 4, + "the appended TAIL window is not a new look", + ), ], ) def test_where_the_independence_bound_falls(starts, kept, why): From c9a206bfffc4bf279958a5e8838b08dde10bc348 Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Tue, 8 Sep 2026 17:25:31 +0200 Subject: [PATCH 2/3] Bound the refusal on independence, not on the hop (review of #51) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The predicate `window_s > clip_duration - hop_s` was a function of `hop_s`, and support-computability is not. Measured against the support `_consensus_alignment` actually returns, it was wrong in both directions: `window_s=11, hop_s=1` on a 15 s clip satisfied it and still returned `support=None` — #43 surviving the fix — while `window_s=6, hop_s=12` was refused although it measures `support=1.0`. The hop drops out because `_window_offsets` appends a tail window at `len(clip) - n_win` whenever the regular grid stops short, so the last window always begins one window's length from the clip's end whatever the hop. A second independent window therefore exists iff `clip_duration - window >= MAX_SUPPORT_OVERLAP * window`. That rule is now `_holds_a_second_look`, and the ceiling `_max_supportable_window_s`. Both compute in SAMPLES, mirroring the rounding the grid is built from. In seconds the ceiling is inadmissible at some durations: on a 10 s clip at 16 kHz `10 / 1.5` rounds up to 106667 samples where 106666 is the most that fits, so a caller retrying at the window the error named would land straight back in the exception. The message rounds its advice down for the same reason. `max_window_s` is now always positive, so the impossible-ceiling branch is gone. Tests: the guard's verdict is checked against `_window_offsets` + `_independent_windows` over a grid of durations, windows and hops rather than against the formula restated; both counterexamples are pinned at unit and support level; the ceiling is asserted admissible (and one sample past it refused) across eight durations; the remedy test now asserts `support is not None`, not merely that the retry is accepted. One further test converted: `window_s=9.5, hop_s=0.5` on a 10 s clip — the muvid#59 shared-bias case — is above the corrected bound, which is exactly what the refusal is for. Claude-Session: https://claude.ai/code/session_01XY8uj3B4XKHKCqbfBRxgeX --- .claude/skills/mixing-audio/SKILL.md | 2 +- mixing/audio/audio_ops.py | 125 ++++++++++++++++++------ mixing/errors.py | 49 ++++++---- mixing/tests/test_adaptive_window.py | 141 ++++++++++++++++++++++----- mixing/tests/test_aligned_spans.py | 64 ++++++++++-- 5 files changed, 300 insertions(+), 81 deletions(-) diff --git a/.claude/skills/mixing-audio/SKILL.md b/.claude/skills/mixing-audio/SKILL.md index 434bb98..c5e320c 100644 --- a/.claude/skills/mixing-audio/SKILL.md +++ b/.claude/skills/mixing-audio/SKILL.md @@ -240,7 +240,7 @@ that spread but does not remove it. Scale your threshold to `window_s`, or pass `window_s` to put every clip on one scale. `aligned_spans` is NOT adapted — there `window_s` is boundary resolution, which is the caller's to choose. -**An explicit `window_s` wider than `clip_duration - hop_s` raises `WindowTooWideForClip`** (0.0.53, issue #43). Above that the clip holds a single window, so the vote you asked for cannot be held: before, you got one whole-clip correlation and a `support=None` that looked exactly like a clip too short to support at any window. The error is importable from `mixing` or `mixing.errors`, subclasses `ValueError`, and carries `clip_index`, `clip_duration_s`, `window_s`, `hop_s` and `max_window_s` — the largest window that still leaves a second look, so a caller putting a mixed-length set on one scale can retry at a window every clip can hold. `max_window_s <= 0` means the clip is shorter than the hop and no window works. This is the explicit path only: `window_s=None` still fits the window to each clip and answers every clip, including one that lands on a single window at the floor. +**An explicit `window_s` a clip cannot hold raises `WindowTooWideForClip`** (0.0.53, issue #43). The bound is `clip_duration / 1.5` — `clip_duration / (1 + MAX_SUPPORT_OVERLAP)` — and **it does not involve `hop_s`**: the analysis grid always appends a tail window one window's length from the clip's end, so a second look exists or not regardless of the hop. (The intuitive `duration - hop_s` is wrong both ways: `window_s=11, hop_s=1` on a 15 s clip satisfies it and still returns `support=None`; `window_s=6, hop_s=12` violates it and measures `support=1.0`.) Above the bound you used to get one whole-clip correlation and a `support=None` that looked exactly like a clip too short to support at any window. The error is importable from `mixing` or `mixing.errors`, subclasses `ValueError`, and carries `clip_index`, `clip_duration_s`, `window_s`, `hop_s` and `max_window_s` — the largest window that still leaves a second look, computed in samples so a retry at exactly that value is guaranteed to measure. This is the explicit path only: `window_s=None` still fits the window to each clip and answers every clip, including one that lands on a single window at the floor. **Reading `window_s` is necessary and, since 0.0.49, no longer sufficient.** A rule of the form *"below the default window, treat the value as unmeasured"* was a complete defence diff --git a/mixing/audio/audio_ops.py b/mixing/audio/audio_ops.py index 0c62439..312179b 100644 --- a/mixing/audio/audio_ops.py +++ b/mixing/audio/audio_ops.py @@ -2128,6 +2128,70 @@ class ClipAlignment: hop_s: "float | None" = None +def _window_samples(seconds: float, sample_rate: int) -> int: + """Samples in a window of ``seconds`` — :func:`_window_offsets`' own rounding. + + Duplicated deliberately rather than shared: the guard must decide on the SAME + integer the grid will be built from, and a guard that rounds even slightly + differently is a guard that disagrees with the thing it guards. + """ + return max(1, int(round(seconds * sample_rate))) + + +def _max_supportable_window_s( + clip_duration_s: float, + sample_rate: int, + *, + max_overlap: float = MAX_SUPPORT_OVERLAP, +) -> float: + """The widest window at which a clip of this length still holds a SECOND look. + + Roughly ``clip_duration_s / (1 + max_overlap)``, and it does NOT depend on ``hop_s`` — + which is the whole subtlety. A hop-based bound (``duration - hop``) is wrong in both + directions: at ``window=11, hop=1`` on a 15 s clip it accepts a call that returns + ``support=None``, and at ``window=6, hop=12`` it refuses one that measures + ``support=1.0``. + + The reason the hop drops out is :func:`_window_offsets`. Its regular grid stops at the + last start that fits, and it then APPENDS a tail window starting at + ``len(clip) - n_win`` whenever the grid did not already reach the end — so whatever + the hop, the last window begins exactly one window's length from the clip's end. + :func:`_independent_windows` keeps a second window once it has moved on by + ``1 - max_overlap`` of a window, so a quorum exists iff + ``clip_duration - window >= max_overlap * window``. The hop decides how MANY looks + there are and how finely a boundary is placed, never whether a second independent one + can exist at all. + + **Computed in SAMPLES, which is not a detail.** The grid rounds the window to whole + samples before any of this happens, and at the seconds bound that rounding can go the + wrong way by half a sample: on a 10 s clip at 16 kHz the exact ``10 / 1.5`` rounds to + 106667 samples where 106666 is the most that fits, so a caller retrying at the + ceiling this function is FOR would land straight back in the exception. Taking the + floor in samples makes the bound admissible by construction — the value returned is + always measurable, which is the only thing that makes it worth reporting. + """ + clip_samples = int(round(clip_duration_s * sample_rate)) + max_window_samples = int(clip_samples / (1.0 + max_overlap)) + return max(1, max_window_samples) / sample_rate + + +def _holds_a_second_look( + clip_duration_s: float, + window_s: float, + sample_rate: int, + *, + max_overlap: float = MAX_SUPPORT_OVERLAP, +) -> bool: + """Whether this window leaves the clip a second INDEPENDENT look. + + :func:`_independent_windows`' own test, on the two windows that always exist: the one + at the start and the one ending at the clip's end. + """ + clip_samples = int(round(clip_duration_s * sample_rate)) + window_samples = _window_samples(window_s, sample_rate) + return clip_samples - window_samples >= max_overlap * window_samples + + def _clip_window_and_hop( clip_duration_s: float, sample_rate: int, @@ -2177,37 +2241,31 @@ def _clip_window_and_hop( support must know that the number arrives on the clip's scale, not the default's. **An EXPLICIT window too wide for the clip is refused, not honored** (issue #43). - Above ``clip_duration_s - hop_s`` the clip holds one window, so the vote the caller - asked for cannot be held and ``support`` comes back ``None`` — which reads exactly - like a clip too short to support at any window. Rather than degrade silently, this - raises :class:`~mixing.errors.WindowTooWideForClip`, naming the clip, its duration - and the largest window that still leaves a second look. Only an explicit window is - refused: the ``None`` path above fits the window to the clip and, at the floor, may - legitimately land on a single window, which is the honest answer for a clip that - short and not a caller's mistake. + Above :func:`_max_supportable_window_s` the clip cannot hold two INDEPENDENT windows + (:func:`_holds_a_second_look`), + so the vote the caller asked for cannot be held and ``support`` comes back ``None`` — + which reads exactly like a clip too short to support at any window. Rather than + degrade silently, this raises :class:`~mixing.errors.WindowTooWideForClip`, naming the + clip, its duration and the largest window that would still hold a quorum. Only an + explicit window is refused: the ``None`` path above fits the window to the clip and, + at the floor, may legitimately land on a single window, which is the honest answer for + a clip that short and not a caller's mistake. """ explicit_window = window_s is not None if window_s is None: floor_s = min_frames * envelope_hop / sample_rate window_s = min(default_window_s, max(floor_s, clip_duration_s / target_windows)) - explicit_hop = hop_s is not None if hop_s is None: hop_s = window_s * hop_ratio - if explicit_window and window_s > clip_duration_s - hop_s: - # The hop is what a second window costs, so the largest window that still - # leaves room for one is `duration - hop` when the hop is the caller's, and - # `duration / (1 + ratio)` when the hop rides on the window. - max_window_s = ( - clip_duration_s - hop_s - if explicit_hop - else clip_duration_s / (1.0 + hop_ratio) - ) + if explicit_window and not _holds_a_second_look( + clip_duration_s, window_s, sample_rate + ): raise WindowTooWideForClip( clip_index=clip_index, clip_duration_s=clip_duration_s, window_s=window_s, hop_s=hop_s, - max_window_s=max_window_s, + max_window_s=_max_supportable_window_s(clip_duration_s, sample_rate), ) return float(window_s), float(hop_s) @@ -2291,15 +2349,17 @@ def align_clips_to_reference( clip's answer moves. Pass a number to fix the window yourself — an explicit value is never overridden. - **An explicit window wider than ``clip_duration - hop_s`` is REFUSED** with - :class:`~mixing.errors.WindowTooWideForClip` (issue #43). Above that length - the clip holds a single window, so the vote asked for cannot be held and the - result would be one whole-clip correlation reporting ``support=None`` — the - same thing a clip too short to support at any window reports, with nothing to - tell the two apart. The error names the clip, its duration and the largest - window that still leaves a second look. To measure such a clip, pass that - window, pass ``window_s=None`` to fit the window to each clip, or pass - ``consensus=False`` to ask for the single correlation outright. + **An explicit window a clip cannot hold is REFUSED** with + :class:`~mixing.errors.WindowTooWideForClip` (issue #43). The bound is + ``clip_duration / (1 + MAX_SUPPORT_OVERLAP)`` and does NOT involve ``hop_s`` + (:func:`_max_supportable_window_s`): above it no second INDEPENDENT window + exists, so the vote asked for cannot be held and the result would be one + whole-clip correlation reporting ``support=None`` — the same thing a clip too + short to support at any window reports, with nothing to tell the two apart. + The error names the clip, its duration and the largest window that still + leaves a second look, and that window is guaranteed measurable. To measure + such a clip, pass it, pass ``window_s=None`` to fit the window to each clip, + or pass ``consensus=False`` to ask for the single correlation outright. hop_s: Step between those windows. Ignored when ``consensus`` is False. ``None`` (the default) is :data:`ADAPTIVE_HOP_RATIO` of whatever window is in force — half of it, the default pair's own ratio — so an adapted grid keeps the @@ -2321,6 +2381,15 @@ def align_clips_to_reference( Returns: A list of :class:`ClipAlignment`, in input order (minus dropped clips). + + Raises: + ValueError: ``feature`` is not one of :data:`ALIGNMENT_FEATURES`, or + ``near_tie_ratio`` is negative. + ~mixing.errors.WindowTooWideForClip: ``consensus`` is on and an EXPLICIT + ``window_s`` is wider than some clip can hold a second independent look at + (issue #43). The error names that clip, its duration and the largest window + that would work; it subclasses ``ValueError``. Never raised on the + ``window_s=None`` path, which fits the window to each clip. """ if feature not in ALIGNMENT_FEATURES: raise ValueError( diff --git a/mixing/errors.py b/mixing/errors.py index 81ce848..97da9fd 100644 --- a/mixing/errors.py +++ b/mixing/errors.py @@ -9,8 +9,15 @@ than parse prose. """ +import math + __all__ = ["MixingError", "WindowTooWideForClip"] +#: Decimal places the remedy in a :class:`WindowTooWideForClip` message is stated to. +#: The number is rounded DOWN to them, so what the message tells a caller to pass is +#: always inside the bound rather than one rounding step outside it. +_ADVICE_DECIMALS = 6 + class MixingError(Exception): """Base for every error `mixing` raises on its own behalf.""" @@ -20,8 +27,8 @@ class WindowTooWideForClip(MixingError, ValueError): """An explicit analysis window leaves a clip no second, independent look. Raised by :func:`mixing.audio.align_clips_to_reference` when the caller passes a - ``window_s`` wider than ``clip_duration_s - hop_s`` — the length below which the - clip holds only one window, so the consensus vote has nothing to arbitrate and + ``window_s`` wider than :attr:`max_window_s` — the length above which the clip holds + no second INDEPENDENT window, so the consensus vote has nothing to arbitrate and ``support`` comes back ``None``. Why an error and not a clamp or a silent single window: the caller asked for a @@ -41,10 +48,12 @@ class WindowTooWideForClip(MixingError, ValueError): clip_duration_s: The clip's duration in seconds. window_s: The window that was asked for. hop_s: The hop in force — the caller's, or the one derived from ``window_s``. - max_window_s: The largest window that would still leave this clip a second - look, given ``hop_s``. ``<= 0`` means no window can: the clip is shorter - than the hop, and only ``consensus=False`` (or a smaller ``hop_s``) - applies. + Reported because it is part of the grid that was asked for; it is NOT part + of the bound, which does not depend on it (see + :func:`~mixing.audio.audio_ops._max_supportable_window_s`). + max_window_s: The largest window that still leaves this clip a second, + independent look. Always positive, and the bound is inclusive: a retry at + exactly this window is measurable, which is what makes it worth reporting. """ def __init__( @@ -62,20 +71,20 @@ def __init__( self.hop_s = float(hop_s) self.max_window_s = float(max_window_s) who = "clip" if clip_index is None else f"clip {clip_index}" - if self.max_window_s > 0: - remedy = ( - f"pass window_s <= {self.max_window_s:.3f}, or window_s=None to fit " - f"the window to each clip" - ) - else: - remedy = ( - f"no window holds a second look at hop_s={self.hop_s:.3f}; pass a " - f"smaller hop_s, window_s=None, or consensus=False" - ) + # Rounded DOWN, never to nearest: a ceiling printed even one ULP above the real + # one sends the caller straight back into this exception, which is the one + # remedy the message must not suggest. + advice = math.floor(self.max_window_s * 10**_ADVICE_DECIMALS) / ( + 10**_ADVICE_DECIMALS + ) super().__init__( f"window_s={self.window_s:.3f} is wider than {who} " - f"({self.clip_duration_s:.3f} s) allows at hop_s={self.hop_s:.3f}: the clip " - f"would hold a single window, so the consensus vote has nothing to " - f"arbitrate and support would be None for a reason nothing reports — " - f"{remedy}." + f"({self.clip_duration_s:.3f} s) allows: no second INDEPENDENT window fits, " + f"so the consensus vote has nothing to arbitrate and support would be None " + f"for a reason nothing reports — pass window_s <= " + f"{advice:.{_ADVICE_DECIMALS}f}, or window_s=None to fit the window to each " + f"clip. " + f"(hop_s={self.hop_s:.3f} does not enter this bound: the analysis grid " + f"always ends with a window one window's length from the clip's end, " + f"whatever the hop.)" ) diff --git a/mixing/tests/test_adaptive_window.py b/mixing/tests/test_adaptive_window.py index a2c349f..0487bea 100644 --- a/mixing/tests/test_adaptive_window.py +++ b/mixing/tests/test_adaptive_window.py @@ -26,6 +26,7 @@ from mixing.errors import WindowTooWideForClip from mixing.audio.audio_ops import ( ADAPTIVE_HOP_RATIO, + MAX_SUPPORT_OVERLAP, ADAPTIVE_WINDOW_MIN_FRAMES, ENVELOPE_HOP, MIN_WINDOWS_FOR_SUPPORT, @@ -183,37 +184,131 @@ def test_an_explicit_window_is_never_overridden(): def test_an_explicit_window_the_clip_cannot_hold_is_refused(): """Issue #43: the boundary of "never overridden" is a refusal, not a silent fallback. - Above ``clip_duration - hop_s`` the clip holds one window, so the vote the caller - named cannot be held. Neither honoring it (one correlation, ``support=None``, no - reason given) nor clamping it (a support on a scale nobody asked for) is reportable, - so the call refuses and the error carries the largest window that would work. - - The bound is exact and inclusive: at ``window_s == clip_duration - hop_s`` a second - window still starts a hop in, so it is accepted. + Above :func:`_max_supportable_window_s` the clip holds no second INDEPENDENT window, + so the vote the caller named cannot be held. Neither honoring it (one correlation, + ``support=None``, no reason given) nor clamping it (a support on a scale nobody asked + for) is reportable, so the call refuses and the error carries the largest window that + would work. + + **The bound is a function of the clip and the overlap rule, NOT of the hop.** A + hop-based bound is the obvious guess and it is wrong in both directions — the two + counterexamples below were measured against the support + ``align_clips_to_reference`` actually returns, and they are pinned here because the + guess is what a future reader will reach for again. """ - assert _clip_window_and_hop(30.0, SR, window_s=20.0, hop_s=10.0) == (20.0, 10.0) + supportable = 15.0 / (1 + MAX_SUPPORT_OVERLAP) # 10.0 s + + # At the bound and below: honored exactly as given, whatever the hop. + assert _clip_window_and_hop(15.0, SR, window_s=supportable, hop_s=3.0) == ( + supportable, + 3.0, + ) with pytest.raises(WindowTooWideForClip) as excinfo: - _clip_window_and_hop(29.0, SR, window_s=20.0, hop_s=10.0, clip_index=3) + _clip_window_and_hop(15.0, SR, window_s=11.0, hop_s=1.0, clip_index=3) err = excinfo.value - assert (err.clip_index, err.window_s, err.hop_s) == (3, 20.0, 10.0) - assert err.clip_duration_s == 29.0 - assert err.max_window_s == pytest.approx(19.0), "duration - the caller's hop" + assert (err.clip_index, err.window_s, err.hop_s) == (3, 11.0, 1.0) + assert err.clip_duration_s == 15.0 + assert err.max_window_s == pytest.approx(supportable) assert "clip 3" in str(err) - # With the hop riding on the window, a wider window buys its own wider hop, so the - # ceiling is duration / (1 + ratio) rather than duration - hop. + # Counterexample 1 — a TINY hop does not buy a second look. `window_s=11, hop_s=1` + # on a 15 s clip passes `window <= duration - hop` and returns support=None: the + # windows the small hop produces overlap far too much to count as second opinions. + # (Same call as above; stated separately because this is what it is pinning.) + assert 11.0 <= 15.0 - 1.0, "the hop-based bound would have accepted this" + + # Counterexample 2 — a hop LONGER than the clip does not cost one. `window_s=6, + # hop_s=12` on a 15 s clip fails `window <= duration - hop`, yet measures + # support=1.0: the grid always appends a tail window at `len(clip) - n_win`, so a + # second look exists no matter how large the hop. + assert 6.0 > 15.0 - 12.0, "the hop-based bound would have refused this" + assert _clip_window_and_hop(15.0, SR, window_s=6.0, hop_s=12.0) == (6.0, 12.0) + + # The hop does not enter the ceiling either — same clip, same window, four hops. + ceilings = { + _clip_window_and_hop_error(15.0, window_s=14.0, hop_s=h).max_window_s + for h in (0.5, 3.0, 7.0, 30.0, None) + } + assert len(ceilings) == 1 and ceilings.pop() == pytest.approx(supportable) + + +def _clip_window_and_hop_error(clip_duration_s, **kwargs): + """The :class:`WindowTooWideForClip` a refused call raises — a test-local helper.""" with pytest.raises(WindowTooWideForClip) as excinfo: - _clip_window_and_hop(24.0, SR, window_s=20.0, hop_s=None) - assert excinfo.value.max_window_s == pytest.approx(24.0 / (1 + ADAPTIVE_HOP_RATIO)) - assert excinfo.value.clip_index is None and "clip (" in str(excinfo.value) + _clip_window_and_hop(clip_duration_s, SR, **kwargs) + return excinfo.value - # Shorter than the hop itself: no window at all can hold a second look, and the - # message must say that rather than name an impossible ceiling. - with pytest.raises(WindowTooWideForClip) as excinfo: - _clip_window_and_hop(8.0, SR, window_s=20.0, hop_s=10.0) - assert excinfo.value.max_window_s <= 0 - assert "consensus=False" in str(excinfo.value) + +def test_the_refused_window_is_exactly_the_one_that_would_have_no_support(): + """The guard's verdict must equal what the estimator would actually report. + + A guard that is merely *nearly* right is worse than none: it refuses calls that + would have measured a support (a false refusal the caller cannot work around) and + lets through calls that return the unexplained ``None`` issue #43 is about. So the + predicate is not an approximation of the rule — it IS the rule + (:func:`_independent_windows`' own test on the two windows that always exist), + evaluated on the same rounded sample counts the grid is built from. + + Checked here against :func:`_window_offsets` + :func:`_independent_windows` + directly, over a grid of durations, windows and hops, rather than against the + formula restated. + """ + for clip_duration_s in (6.0, 10.0, 12.5, 15.0, 22.0): + reference = np.zeros(int(90 * SR)) + clip = np.zeros(int(clip_duration_s * SR)) + for window_s in (1.0, 3.33, 5.0, 7.0, 9.0, 11.0, 20.0): + for hop_s in (0.5, 2.0, 8.0, 30.0): + windows = _window_offsets( + reference, + clip, + SR, + window_s=window_s, + hop_s=hop_s, + feature="envelope", + min_overlap_ratio=0.5, + ) + would_support = ( + len(_independent_windows(windows)) >= MIN_WINDOWS_FOR_SUPPORT + ) + try: + _clip_window_and_hop( + clip_duration_s, SR, window_s=window_s, hop_s=hop_s + ) + accepted = True + except WindowTooWideForClip: + accepted = False + assert accepted is would_support, ( + f"guard says {accepted} but {clip_duration_s} s at " + f"window_s={window_s}, hop_s={hop_s} would " + f"{'have' if would_support else 'have no'} support" + ) + + +def test_the_ceiling_the_error_names_is_one_the_call_accepts(): + """The remedy must work, and half a sample of rounding is enough to break it. + + ``max_window_s`` exists to be retried at, so a value even one rounding step outside + the bound sends the caller straight back into the exception. The seconds expression + does exactly that at some durations: on a 10 s clip at 16 kHz, ``10 / 1.5`` rounds + UP to 106667 samples where 106666 is the most that fits. The bound is therefore + taken in samples, and this pins the property rather than the expression. + """ + for clip_duration_s in (6.0, 8.0, 10.0, 12.5, 15.0, 22.0, 30.0, 47.3): + err = _clip_window_and_hop_error( + clip_duration_s, window_s=clip_duration_s, hop_s=1.0 + ) + assert err.max_window_s > 0, "some window always holds a second look" + # The ceiling itself is accepted... + _clip_window_and_hop(clip_duration_s, SR, window_s=err.max_window_s, hop_s=1.0) + # ...and it really is the ceiling: one sample more is not. + with pytest.raises(WindowTooWideForClip): + _clip_window_and_hop( + clip_duration_s, SR, window_s=err.max_window_s + 1 / SR, hop_s=1.0 + ) + # The number the MESSAGE names is rounded down, so it is admissible too. + advised = float(str(err).split("pass window_s <= ")[1].split(",")[0]) + _clip_window_and_hop(clip_duration_s, SR, window_s=advised, hop_s=1.0) #: The smallest clip that can hold two half-window-separated looks at the floor window: diff --git a/mixing/tests/test_aligned_spans.py b/mixing/tests/test_aligned_spans.py index 21396a7..ac7b7d7 100644 --- a/mixing/tests/test_aligned_spans.py +++ b/mixing/tests/test_aligned_spans.py @@ -694,18 +694,53 @@ def test_an_explicit_window_wider_than_the_clip_is_refused(song, ref, tmp_path): assert err.clip_index == 0 assert err.window_s == SPAN_WINDOW_S and err.hop_s == SPAN_HOP_S assert err.clip_duration_s == pytest.approx(15.0, abs=0.05) - # duration - hop: the largest window that still leaves room for a second look. - assert err.max_window_s == pytest.approx(5.0, abs=0.05) + # duration / (1 + MAX_SUPPORT_OVERLAP): the largest window that still leaves a + # second INDEPENDENT look. NOT duration - hop_s — see the counterexamples in + # test_adaptive_window.py, which the hop-based bound gets wrong in both directions. + assert err.max_window_s == pytest.approx(10.0, abs=0.05) # The numbers must be IN the message too — the point of the error is that the # window, not the clip's length alone, is nameable as the reason. text = str(err) assert "window_s=20.000" in text and "clip 0" in text assert isinstance(err, ValueError), "existing ValueError handlers keep working" - # And the window the error names is one the call accepts. + # And the window the error names must RESTORE what the refusal withheld: not merely + # be accepted, but come back with the support the caller asked for in the first + # place. An accepted-but-still-unmeasured ceiling would be the original bug wearing + # an exception. (ok,) = align_clips_to_reference( song, [clip], sample_rate=SR, window_s=err.max_window_s, hop_s=SPAN_HOP_S ) assert ok.window_s == pytest.approx(err.max_window_s) + assert ok.support is not None, "the remedy the error names must actually measure" + + +def test_the_bound_is_the_clips_length_and_the_overlap_rule_not_the_hop( + song, ref, tmp_path +): + """The two hop-based counterexamples, stated on real support rather than arithmetic. + + ``window_s > clip_duration - hop_s`` is the intuitive bound and it is wrong in both + directions, because :func:`_window_offsets` appends a tail window at + ``len(clip) - n_win`` whatever the hop: + + - a TINY hop does not buy a second look — ``window_s=11, hop_s=1`` on a 15 s clip + satisfies the hop-based bound and still has nothing independent to vote with; + - a hop LONGER than the clip does not cost one — ``window_s=6, hop_s=12`` violates + the hop-based bound and measures a support perfectly well. + + Pinned on the estimator's own output, so the guard cannot drift back onto the + intuitive rule without a test failing on the thing that actually matters. + """ + rng = np.random.default_rng(41) + clip = _write(tmp_path, "fifteen.wav", _take(ref, 30, 45, rng)) + + with pytest.raises(WindowTooWideForClip): + align_clips_to_reference(song, [clip], sample_rate=SR, window_s=11.0, hop_s=1.0) + + (wide_hop,) = align_clips_to_reference( + song, [clip], sample_rate=SR, window_s=6.0, hop_s=12.0 + ) + assert wide_hop.support is not None, "the tail window is a second look" # -------------------------------------------------------------------------- @@ -742,7 +777,7 @@ def test_the_one_window_reproduction_is_now_a_refusal(two_halves, tmp_path): align_clips_to_reference( song, [clip], sample_rate=SR, window_s=SPAN_WINDOW_S, hop_s=SPAN_HOP_S ) - assert excinfo.value.max_window_s == pytest.approx(5.0, abs=0.05) + assert excinfo.value.max_window_s == pytest.approx(10.0, abs=0.05) (single,) = align_clips_to_reference(song, [clip], sample_rate=SR, consensus=False) assert single.offset_s == 75.0, ( @@ -843,17 +878,28 @@ def test_windows_that_are_the_same_look_twice_do_not_count_as_support( :data:`~mixing.audio.audio_ops.MAX_SUPPORT_OVERLAP`, and below a quorum of those the answer is ``None`` — unmeasured — rather than a manufactured 1.0. - The offset itself is unaffected: every window still votes. + Since issue #43 this exact call does not return an unmeasured answer — it REFUSES. + ``window_s=9.5`` on a 10 s clip is above the largest window that leaves a second + independent look (10 / 1.5 = 6.67), so the vote it names cannot be held, and the + ``support=None`` this test was written to pin is precisely the unexplained silence + #43 is about. The tally rule is unchanged and still what makes the answer ``None``; + what changed is that the caller is now told, and told at what window to ask again. + + The offset itself is unaffected: every window still votes, which the retry shows. """ rng = np.random.default_rng(41) clip = _write(tmp_path, "overlapped.wav", _take(ref, 20, 30, rng)) - (crowded,) = align_clips_to_reference( - song, [clip], sample_rate=SR, window_s=9.5, hop_s=0.5 + with pytest.raises(WindowTooWideForClip) as excinfo: + align_clips_to_reference(song, [clip], sample_rate=SR, window_s=9.5, hop_s=0.5) + assert excinfo.value.max_window_s == pytest.approx(10.0 / 1.5, abs=0.05) + + (retried,) = align_clips_to_reference( + song, [clip], sample_rate=SR, window_s=excinfo.value.max_window_s, hop_s=0.5 ) - assert crowded.offset_s == pytest.approx(20.0, abs=0.05), ( + assert retried.offset_s == pytest.approx(20.0, abs=0.05), ( "the offset still measures" ) - assert crowded.support is None, "but 95%-overlapping windows are not two opinions" + assert retried.support is not None, "and now it is corroborated as well" (spaced,) = align_clips_to_reference( song, [clip], sample_rate=SR, window_s=5.0, hop_s=2.5 From 4b1484886a90ece574ef3de865f4dcd49020a22a Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Tue, 8 Sep 2026 21:24:25 +0200 Subject: [PATCH 3/3] Review nits: honest ceiling claim, boundary cell (#51) Two documentation claims said the ceiling is always positive and measurable. Measured: true from a 2-sample clip up, false for a 1-sample one, where no window holds a second look at all and the value is floored at one sample. Reworded as a lower bound in that degenerate case rather than a promise. The guard/engine equivalence grid had no cell at the bound itself, which is the only place an off-by-one in either direction shows. Added `duration / 1.5`. Claude-Session: https://claude.ai/code/session_01XY8uj3B4XKHKCqbfBRxgeX --- mixing/audio/audio_ops.py | 14 +++++++++----- mixing/errors.py | 7 +++++-- mixing/tests/test_adaptive_window.py | 5 ++++- 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/mixing/audio/audio_ops.py b/mixing/audio/audio_ops.py index 312179b..a602f86 100644 --- a/mixing/audio/audio_ops.py +++ b/mixing/audio/audio_ops.py @@ -2167,8 +2167,10 @@ def _max_supportable_window_s( wrong way by half a sample: on a 10 s clip at 16 kHz the exact ``10 / 1.5`` rounds to 106667 samples where 106666 is the most that fits, so a caller retrying at the ceiling this function is FOR would land straight back in the exception. Taking the - floor in samples makes the bound admissible by construction — the value returned is - always measurable, which is the only thing that makes it worth reporting. + floor in samples makes the bound admissible by construction wherever one exists, + which is the only thing that makes it worth reporting. The exception is a degenerate + clip of a sample or two, which holds no second look at ANY window: the result is + floored at one sample there and is a lower bound rather than a promise. """ clip_samples = int(round(clip_duration_s * sample_rate)) max_window_samples = int(clip_samples / (1.0 + max_overlap)) @@ -2357,9 +2359,11 @@ def align_clips_to_reference( whole-clip correlation reporting ``support=None`` — the same thing a clip too short to support at any window reports, with nothing to tell the two apart. The error names the clip, its duration and the largest window that still - leaves a second look, and that window is guaranteed measurable. To measure - such a clip, pass it, pass ``window_s=None`` to fit the window to each clip, - or pass ``consensus=False`` to ask for the single correlation outright. + leaves a second look — measurable for any clip long enough to be worth + aligning, though a degenerate clip of a sample or two has no such window at + all. To measure such a clip, pass that window, pass ``window_s=None`` to fit + the window to each clip, or pass ``consensus=False`` to ask for the single + correlation outright. hop_s: Step between those windows. Ignored when ``consensus`` is False. ``None`` (the default) is :data:`ADAPTIVE_HOP_RATIO` of whatever window is in force — half of it, the default pair's own ratio — so an adapted grid keeps the diff --git a/mixing/errors.py b/mixing/errors.py index 97da9fd..4f41677 100644 --- a/mixing/errors.py +++ b/mixing/errors.py @@ -52,8 +52,11 @@ class WindowTooWideForClip(MixingError, ValueError): of the bound, which does not depend on it (see :func:`~mixing.audio.audio_ops._max_supportable_window_s`). max_window_s: The largest window that still leaves this clip a second, - independent look. Always positive, and the bound is inclusive: a retry at - exactly this window is measurable, which is what makes it worth reporting. + independent look. The bound is inclusive, so for any clip long enough to be + worth aligning a retry at exactly this window measures — which is what makes + it worth reporting. A degenerate clip of a sample or two has no such window + at all; the value is floored at one sample there and is a lower bound rather + than a promise. """ def __init__( diff --git a/mixing/tests/test_adaptive_window.py b/mixing/tests/test_adaptive_window.py index 0487bea..1f94b56 100644 --- a/mixing/tests/test_adaptive_window.py +++ b/mixing/tests/test_adaptive_window.py @@ -257,7 +257,10 @@ def test_the_refused_window_is_exactly_the_one_that_would_have_no_support(): for clip_duration_s in (6.0, 10.0, 12.5, 15.0, 22.0): reference = np.zeros(int(90 * SR)) clip = np.zeros(int(clip_duration_s * SR)) - for window_s in (1.0, 3.33, 5.0, 7.0, 9.0, 11.0, 20.0): + # The exact bound is in the list: an off-by-one in either direction shows up + # only at the cell where accepted and refused meet. + boundary_s = clip_duration_s / (1 + MAX_SUPPORT_OVERLAP) + for window_s in (1.0, 3.33, 5.0, boundary_s, 7.0, 9.0, 11.0, 20.0): for hop_s in (0.5, 2.0, 8.0, 30.0): windows = _window_offsets( reference,