Repository navigation
Refuse an explicit window_s a clip cannot hold (#43) - #51
Merged
Merged
Conversation
`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
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
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
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
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.
Closes #43.
What changed
align_clips_to_reference(..., window_s=45)on a 50 s clip was accepted, produced one window, and returnedsupport=None. The caller asked for a windowed vote and got a single whole-clip correlation — and the only signal was the absence of a support number, which is indistinguishable from a clip genuinely too short to support at any window.Above
clip_duration / (1 + MAX_SUPPORT_OVERLAP)the clip holds no second independent window, so the vote asked for cannot be held._clip_window_and_hopnow refuses that case with a typed error.mixing/errors.py(new, pure stdlib) —MixingErrorbase +WindowTooWideForClip. This is the first typed errormixingraises on its own behalf, so the module is also the home for the next one. Reachable asmixing.WindowTooWideForClipthrough the lazy facade (no backend pulled), and re-exported frommixing.audio.clip_index,clip_duration_s,window_s,hop_sandmax_window_s— the largest window that still leaves a second, independent look — so a caller putting a mixed-length set on one scale can retry at a window every clip holds.max_window_sis always positive.The bound does not involve
hop_s(fixed in review, commitc9a206b)The first version of this PR used
window_s > clip_duration - hop_s, which is the intuitive rule and is wrong in both directions. cn-video-gen-2's reviewer measured 18/64 disagreements against the support_consensus_alignmentactually returns; both counterexamples are now pinned as tests:duration - hopboundwindow_s=11, hop_s=1support=None— #43 surviving the fixwindow_s=6, hop_s=12support=1.0— a false refusalThe hop drops out because
_window_offsetsappends a tail window atlen(clip) - n_winwhenever the regular grid stops short, so the last window always begins exactly one window's length from the clip's end, whatever the hop._independent_windowskeeps a second window once it has moved on by1 - MAX_SUPPORT_OVERLAPof a window, so a quorum exists iffclip_duration - window >= MAX_SUPPORT_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.That rule is
_holds_a_second_look; the ceiling is_max_supportable_window_s. Both compute in samples, mirroring_window_offsets' own rounding: in seconds the ceiling is inadmissible at some durations (on a 10 s clip at 16 kHz,10 / 1.5rounds 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.ValueError, soexcept ValueErroraround alignment keeps working._clip_window_and_hop,align_clips_to_reference'swindow_s) and a paragraph added to themixing-audioskill.A call passing an explicit
window_swider thanclip_duration / (1 + MAX_SUPPORT_OVERLAP)— i.e.clip_duration / 1.5, the widest window that still leaves a second independent look — now raises where it previously returned an answer. The bound does not involvehop_s; see the section above. That answer was the uncorroborated single-window one, but it was an answer, and code depending on it will now see an exception. Fleet policy here is typed refusal over silent degradation, as in #45/#47 and muvid#59.The default path is deliberately untouched: with
window_s=Nonethe window is still fitted per clip and every clip is answered, including one that lands on a single window at the floor — that is the estimator doing its best for a short clip, not a caller's mistake. A new test (test_the_default_window_path_is_untouched_by_the_refusal) pins that boundary across a long / fitted / floor-window trio rather than leaving it to be inferred.aligned_spansis unchanged — it does not route through_clip_window_and_hop, and therewindow_smeans boundary resolution rather than vote size (the open half of #43's last bullet).Tests
The issue named two tests pinning the silent contract; there were six in practice — the four in
test_adaptive_window.pyused the wide-window call as their baseline for what issue #41 fixed. All six now assert the refusal, and each keeps the measurement it was pinning:test_aligned_spans.py:test_an_explicit_window_wider_than_the_clip_is_refused(was…takes_the_whole_clip_answer),test_the_one_window_reproduction_is_now_a_refusal(wastest_support_is_None_where_no_second_opinion_exists; theconsensus=Falsehalf is kept as the contrast — asked for explicitly, the single correlation is an answer, not a degradation).test_adaptive_window.py:test_an_explicit_window_is_never_overriddenrestated on a clip the window fits, plus a newtest_an_explicit_window_the_clip_cannot_hold_is_refusedpinning the inclusive bound, both ceiling formulas, and themax_window_s <= 0message; the two#41acceptance tests nowpytest.raiseson their baseline call;test_the_reported_window_is_the_one_asked_for_when_one_is_asked_forgiven a clip long enough to hold the window it pins.Added in review:
test_the_refused_window_is_exactly_the_one_that_would_have_no_support— the guard's verdict checked against_window_offsets+_independent_windowsdirectly over a grid of durations × windows × hops, rather than against the formula restated.test_the_ceiling_the_error_names_is_one_the_call_accepts— across eight durations: the ceiling is accepted, one sample past it is refused, and the number the message names is admissible too.test_the_bound_is_the_clips_length_and_the_overlap_rule_not_the_hop— both counterexamples on real support, so the guard cannot drift back onto the intuitive rule silently.ok.support is not None, not merely that the retry is accepted — an accepted-but-still-unmeasured ceiling would be the original bug wearing an exception.One further test converted in review:
test_windows_that_are_the_same_look_twice_do_not_count_as_supportpasseswindow_s=9.5, hop_s=0.5on a 10 s clip — the muvid#59 shared-bias case — which is above the corrected bound. That call returning an unexplainedsupport=Noneis exactly what the refusal is for, so it now asserts the refusal and retries at the ceiling to show the offset still measures.Out-of-suite verification: a 616-cell grid (7 durations × 11 windows × 8 hops,
hop_s=Noneincluded) comparing the guard's verdict against the supportalign_clips_to_referenceactually returns — 0 misses, 0 false refusals, 0 inadmissible ceilings.611 passed, 1 skipped.
ruff check/ruff formatclean.muvid consumer
Checked read-only. muvid does not need a code change:
muvid.footage.align.align_footagealready refuses to forwardwindow_s/hop_stomixingat its own entry point (WINDOW_PARAMETERS, muvid#59), so no muvid call path can reach this refusal. Its own test asserts aTypeErrorfrom muvid, which still holds.What is now stale is muvid's rationale prose: the
WINDOW_PARAMETERScomment andtest_a_wider_window_is_refused_because_it_would_turn_the_gate_offjustify muvid's refusal partly by "window_s=45on a 50 s clip givessupport=None, i.e. the gate switches off silently" — whichmixingno longer does. muvid's refusal still stands on its other leg (narrowing the window deflates every support), so this is a docs-accuracy follow-up, not a break. Filed as a discovered issue: reeleehq/muvid#98.https://claude.ai/code/session_01XY8uj3B4XKHKCqbfBRxgeX