fix(gp_autosync): slope-constrained DTW steps — stop path collapse on riff-based songs - #791
Conversation
… riff-based songs librosa.sequence.dtw's default step sizes permit pure horizontal/vertical moves; on songs whose chroma is self-similar for long stretches the flat cost surface let the path collapse (minutes of score onto one audio frame), so auto-sync produced monotonic-but-garbage sync points and the per-bar warp imported charts badly out of sync while reporting success. Use the standard music-sync step pattern [[1,1],[1,2],[2,1]] (local tempo ratio bounded to 0.5x-2x), falling back to unconstrained steps if the global length ratio makes it infeasible. Validated on the reported song (138 BPM tab, YouTube audio): coarse points now track 1:1, refine holds slopes 0.77-1.04, warped downbeats hit onset peaks at 3.3x background energy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe change modifies ChangesDTW auto-sync fix
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant DtwAlign as _dtw_align
participant Librosa as librosa.sequence.dtw
Caller->>DtwAlign: request alignment(score_chroma, audio_chroma)
DtwAlign->>Librosa: dtw(step_sizes_sigma=constrained pattern)
alt constrained DTW succeeds
Librosa-->>DtwAlign: warping path
else constrained DTW infeasible (raises)
DtwAlign->>DtwAlign: log warning
DtwAlign->>Librosa: dtw(unconstrained)
Librosa-->>DtwAlign: warping path
end
DtwAlign-->>Caller: return alignment result
Related issues: None specified. Related PRs: None specified. Suggested labels: bug, audio-sync Suggested reviewers: None specified. 🐰 A hop, a skip, a DTW dance, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR hardens Guitar Pro auto-sync DTW alignment against “path collapse” on riff/self-similar music by switching _dtw_align to a slope-constrained DTW step pattern (with a fallback when the constraint is infeasible), preventing many-bars-to-one-frame mappings that previously produced wildly incorrect per-bar warps while still reporting success.
Changes:
- Update
_dtw_alignto prefer a slope-constrained DTW step pattern ([[1,1],[1,2],[2,1]]) to prevent degenerate DTW paths on flat cost surfaces. - Add a fallback to unconstrained DTW when the constrained pattern cannot be applied.
- Document the fix and its motivation/validation details in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/gp_autosync.py | Uses slope-constrained DTW steps (with fallback) to stop warping-path collapse on riff-based songs. |
| CHANGELOG.md | Adds an Unreleased “Fixed” entry describing the DTW step constraint change and its impact. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| steps = np.array([[1, 1], [1, 2], [2, 1]]) | ||
| weights = np.array([1.0, 1.0, 1.0]) | ||
| try: | ||
| _D, wp = librosa.sequence.dtw( | ||
| cs, ca, metric='cosine', | ||
| step_sizes_sigma=steps, weights_mul=weights, | ||
| ) | ||
| except Exception as exc: | ||
| # The constrained pattern needs the global length ratio within its | ||
| # 0.5x-2x slope bounds; a pathological pairing (e.g. a 3-minute tab | ||
| # against a 20-minute video) is infeasible and librosa raises. Fall | ||
| # back to the unconstrained path rather than failing the whole sync. | ||
| _log.warning("gp_autosync: constrained DTW infeasible (%s) — " | ||
| "falling back to unconstrained steps", exc) | ||
| _D, wp = librosa.sequence.dtw(cs, ca, metric='cosine') |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/gp_autosync.py (1)
559-566: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNarrow the caught exception type.
Ruff BLE001 flags
except Exception. Since the fallback is specifically meant for the constrained pattern's slope-ratio infeasibility, catchinglibrosa.util.exceptions.ParameterError(librosa's documented exception fordtwparameter/dimension issues) would be more precise and avoid silently swallowing unrelated bugs (e.g. aTypeErrorfrom a future signature change).♻️ Suggested narrower exception
+ from librosa.util.exceptions import ParameterError try: _D, wp = librosa.sequence.dtw( cs, ca, metric='cosine', step_sizes_sigma=steps, weights_mul=weights, ) - except Exception as exc: + except ParameterError as exc:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/gp_autosync.py` around lines 559 - 566, The fallback in gp_autosync’s constrained DTW path is catching too broadly with a generic Exception. Narrow the handler in the dtw fallback block to librosa’s documented ParameterError so only slope-ratio / parameter infeasibility triggers the unconstrained retry. Keep the existing warning and fallback call in the same constrained-pattern exception path, but avoid swallowing unrelated errors around librosa.sequence.dtw.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/gp_autosync.py`:
- Around line 559-566: The fallback in gp_autosync’s constrained DTW path is
catching too broadly with a generic Exception. Narrow the handler in the dtw
fallback block to librosa’s documented ParameterError so only slope-ratio /
parameter infeasibility triggers the unconstrained retry. Keep the existing
warning and fallback call in the same constrained-pattern exception path, but
avoid swallowing unrelated errors around librosa.sequence.dtw.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dead38f6-70b2-4c46-9436-8fb42404a51a
📒 Files selected for processing (2)
CHANGELOG.mdlib/gp_autosync.py
What
Follow-up to #787, from Byron's first real-world test (138 BPM tab + YouTube audio): the import reported Imported with per-bar audio sync but the chart was badly out of sync, with the editor showing an effective tempo of 159.4 BPM.
Root cause
_dtw_aligncalledlibrosa.sequence.dtwwith the default step pattern, which permits unbounded horizontal/vertical path runs. On riff-based music (long self-similar chroma stretches — this was a stoner-rock track) the DTW cost surface is nearly flat, and the path collapsed: three sync points shared one audio timestamp, 26s of score mapped onto 8s of audio. The points were monotonic, so the anchor sanity gates kept them, and the warp faithfully applied a garbage mapping.Fix
Use the standard music-sync slope-constrained step pattern
[[1,1],[1,2],[2,1]](every step advances both axes; local tempo ratio bounded to 0.5x–2x), which makes the degenerate path structurally impossible. Falls back to unconstrained steps when the global length ratio is outside the slope bounds (e.g. a 3-minute tab vs a full-concert video) rather than failing the sync.Validation on the failing inputs
tests/test_gp_autosync_warp.py+tests/test_gp_audio_sync.py: 41 passed🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation