Return a most-likely path from forced_align when predecessors tie - #4222
Open
happyarts wants to merge 1 commit into
Open
Return a most-likely path from forced_align when predecessors tie#4222happyarts wants to merge 1 commit into
happyarts wants to merge 1 commit into
Conversation
The per-cell choice in forced_align_impl is
if (x2 > x1 && x2 > x0) { result = x2; backPtr = 2; }
else if (x1 > x0 && x1 > x2) { result = x1; backPtr = 1; }
else { result = x0; backPtr = 0; }
When x1 == x2 and both exceed x0, neither condition holds and the else
branch takes x0 -- the strictly worst of the three. The returned path is
then not a most-likely one, which is the entire contract of the function.
Reaching the second branch already implies that x2 is not strictly greater
than both others, so `x1 > x0` alone is the correct and complete condition
there; `&& x1 > x2` only diverts exact ties into the wrong branch. Both
kernels carry the same chain, so both are corrected.
Exhaustively verified on 3243 randomly generated small cases (T <= 5,
vocabulary <= 4, integer log-probabilities so ties are exact) by comparing
against the best score over all enumerated valid paths: 92 sub-optimal
before, 0 after, worst deficit 3.0 log-probability. The existing
forced_align tests are unaffected, as are 43 recorded alignments from real
wav2vec2 emissions -- on non-degenerate emissions exact ties are rare, which
is presumably why this survived.
The added test is asserted on the score rather than on the path: whenever
this bug can fire, x1 == x2 means two distinct paths reach that cell with
the same score, so the optimum is never unique and the choice among optimal
paths is a convention the test should not pin.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/audio/4222
Note: Links to docs will display an error until the docs builds have been completed. This comment was automatically generated by Dr. CI and updates every 15 minutes. |
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.
Fixes #4221
forced_aligncan return an alignment that is not a most-likely path. The per-cell choice isand when
x1 == x2with both abovex0, neither condition holds, so theelsebranch takesx0— the strictly worst of the three. Both the CPU and the CUDA kernel carry the identical chain, so both are corrected here.Why
x1 > x0alone is correctReaching the second branch already implies
x2is not strictly greater than both others:x2withx2 > x1andx2 > x0— the maximum;x1withx1 > x0; hadx2 > x1held, thenx2 > x1 > x0would have taken branch 1, sox2 ≤ x1— the maximum;x0withx0 ≥ x1; hadx2 > x0held, thenx2 > x0 ≥ x1would have taken branch 1, sox2 ≤ x0— the maximum.So the only behaviour that changes is that exact ties now route to the larger value instead of to
x0.Verification
T ≤ 5, vocabulary≤ 4, integer log-probabilities so ties are exact): 92 sub-optimal before, 0 after, worst deficit 3.0 log-probability.T == L + repeatsfits) are bit-identical before and after, as are the existingtest_forced_alignexpectations.main(4e3e282) withUSE_CUDA=0 USE_ROCM=0 BUILD_SOX=0 USE_FFMPEG=0; the whole oftest/torchaudio_unittest/functional/functional_cpu_test.pypasses — 700 passed, 1 xfailed — including the 4 added cases. The CUDA kernel is changed identically but I have no CUDA device to test on; the added test lives inFunctional, whichfunctional_cuda_test.pyalso instantiates, so CI covers both backends.About the added test
It asserts the score, not the path. Whenever this bug can fire,
x1 == x2means two distinct paths reach that cell with equal score, so an optimum is never unique — which of the equally-good paths is returned is a tie-breaking convention, and pinning it in a test would only make the test brittle.Note
This touches
forced_align_implin the same region as #4209 (32-bit index overflow, approved and unmerged). The two changes are independent — that one replaces the DP buffers, this one changes a condition — but whichever lands second will want a trivial rebase.