Add optional octave-consistency pass (Viterbi per-note octave assignment) - #130
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a new "Octave Consistency" post-processing feature that uses dynamic programming to reassign per-note octaves, correcting scattered wrong-octave notes while preserving genuine wide-range passages. It's wired into the pipeline via a new Settings field, CLI flag, GUI toggle, and documented in README, with accompanying tests. ChangesOctave Consistency Feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant UltraSinger
participant MidiCreator as enforce_octave_consistency
participant MidiSegments
User->>UltraSinger: run with --octave_consistency
UltraSinger->>UltraSinger: snap_isolated_octave_spikes()
UltraSinger->>MidiCreator: enforce_octave_consistency(midi_segments)
MidiCreator->>MidiCreator: convert notes to MIDI, filter invalid
MidiCreator->>MidiCreator: DP selects octave offsets per note
MidiCreator->>MidiSegments: update note assignments
MidiCreator-->>UltraSinger: adjusted midi_segments
UltraSinger->>UltraSinger: correct_vocal_center() (subsequent stage)
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/modules/Midi/midi_creator.py`:
- Around line 559-563: The note conversion fallback in MidiCreator’s segment
loop only handles ValueError and KeyError, so malformed note strings from
librosa.note_to_midi() can still escape. Update the exception handling in the
midi_segments processing block to also catch librosa.ParameterError and continue
appending None for invalid notes, keeping the behavior consistent in
midi_creator.py.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c3841785-bf6d-4dc4-8c2b-acf337656589
📒 Files selected for processing (9)
README.mdpytest/modules/Midi/test_octave_consistency.pysrc/Settings.pysrc/UltraSinger.pysrc/gui/config.pysrc/gui/settings_tab.pysrc/gui/ultrasinger_runner.pysrc/modules/Midi/midi_creator.pysrc/modules/common_print.py
|
Confirmed empirically in the venv: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Problem (field report)
Passages jumping between octaves make generated charts extremely confusing to sing. A dedicated experiment against the 8 professional reference songs quantified it: the references contain 47 adjacent note jumps of >=10 semitones in total — our generated charts contain 507 (10x). The errors are not only whole passages: they are scattered single notes, short runs, and in one song a rapid octave alternation across whole sections.
Investigation highlights
Change
--octave_consistency(GUI: Post-Processing → "Octave Consistency", off by default): keeps every note's pitch class and re-chooses only its octave (-1/0/+1) via dynamic programming over the whole song. Cost = hinge smoothness (only the part of an adjacent jump beyond 8 semitones costs) + fidelity (2 per octave moved). The balance is self-limiting: folding 1-3 note scatter is cheaper than its two ~12-semitone boundary jumps, while a genuine octave passage of >= ~5 notes is cheaper to keep — validated:Genuine wide-range songs unaffected (the widest ref, 42 st, keeps its match% and structure). The game score is unchanged (ptAKF folds octaves). The implemented pipeline function reproduces the prototype numbers exactly on all 8 songs, and pitch classes are asserted preserved.
Surfaces (kept in sync)
CLI flag + help, GUI toggle with tooltip, config default, runner arg, README section (next to the existing octave-shift/octave-snap docs, explaining the relationship: snap = single isolated spikes, consistency = the stronger scatter/short-run repair).
Tests
7 unit tests: single spike folded, short run folded, alternating scatter unified, long genuine octave passage kept, gradual wide range untouched, pitch class preserved, short-input no-op. Full suite 873 passed / 4 skipped.
Default: off (opt-in) — the default can be revisited after real singing tests.
Summary by CodeRabbit
New Features
Bug Fixes