Add vocal-centre safety-net for consistent wrong-octave detection - #29
Conversation
When ALL detected pitches are consistently in the wrong octave (e.g. -12 HT), correct_global_octave() may not catch it because the median still falls inside the broad [48,84] range, and correct_octave_outliers() can't help because there are no outliers to compare against (100% consistency = "correct"). The new correct_vocal_center() function runs AFTER both existing corrections as a final safety net. It uses a tighter expected centre band (MIDI 55-79, roughly G3-G5) and checks if >80% of notes cluster below/above the threshold. If so, it shifts all notes by ±12 semitones. Pipeline order: global_octave → octave_outliers → vocal_center → manual_shift Includes 13 unit tests covering edge cases, boundary conditions, concentration thresholds, interval preservation, and custom parameters. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA new correct_vocal_center function was added to the MIDI module to shift note collections by octaves when concentrated outside vocal thresholds. It's wired into UltraSinger's processing pipeline behind a settings flag and covered by extensive new tests. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Settings as Settings
participant Runner as UltraSinger.run
participant MidiMod as modules.Midi.midi_creator
participant Segs as MidiSegments
CLI->>Settings: parse args (--disable_vocal_center)
Settings->>Runner: provide config (vocal_center_correction)
Runner->>MidiMod: process_data.midi_segments (after octave-outlier correction)
alt vocal_center_correction == true
MidiMod->>Segs: correct_vocal_center(segs, thresholds)
Segs-->>MidiMod: possibly shifted segments
end
Runner->>Segs: continue pipeline with final segments
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pytest/modules/Midi/test_midi_creator.py (1)
524-617: Add a no-shift regression for valid low/high-centre input.These tests only prove the “shift” path. A correct passage like
["F#3"] * 5is still inside the broader 48-84 vocal range, but this heuristic would currently treat it as wrong-octave, and the suite has no case here to catch that false positive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pytest/modules/Midi/test_midi_creator.py` around lines 524 - 617, Add a regression test that ensures correct_vocal_center does NOT shift notes that are already in a valid mid-range (e.g., five identical "F#3" notes); create a new test (e.g., test_no_shift_valid_center) that builds segs with self._make_segs(["F#3"] * 5), calls correct_vocal_center(segs), extracts midis with self._get_midis(result) and asserts the midis are unchanged (expected = original MIDI values for F#3), so the heuristic won't falsely octave-shift valid input.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/Midi/midi_creator.py`:
- Around line 249-252: The concentration check currently uses a >= comparator
which causes an exact-threshold case to trigger an octave shift contrary to the
docstring; change the three comparisons that reference concentration_pct (the
expressions counting notes below low_threshold or above high_threshold and
comparing to concentration_pct) to use > instead of >= so the shift only occurs
when more than concentration_pct of notes are outside the centre band (update
all occurrences that reference concentration_pct, low_threshold, and
high_threshold).
In `@src/UltraSinger.py`:
- Around line 239-241: The unconditional call to correct_vocal_center on
process_data.midi_segments inside run() can transpose already-correct tracks;
change it to only run when a configurable flag or stronger trigger is set (e.g.,
add a boolean option like enable_vocal_center_correction or a validation
function that checks extreme out-of-range concentration before calling
correct_vocal_center), wire that flag into the run() flow so
correct_vocal_center(process_data.midi_segments) is executed only when the
flag/trigger is true, and ensure the new option is documented and defaults to
false to avoid unexpected full-track transpositions.
---
Nitpick comments:
In `@pytest/modules/Midi/test_midi_creator.py`:
- Around line 524-617: Add a regression test that ensures correct_vocal_center
does NOT shift notes that are already in a valid mid-range (e.g., five identical
"F#3" notes); create a new test (e.g., test_no_shift_valid_center) that builds
segs with self._make_segs(["F#3"] * 5), calls correct_vocal_center(segs),
extracts midis with self._get_midis(result) and asserts the midis are unchanged
(expected = original MIDI values for F#3), so the heuristic won't falsely
octave-shift valid input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ad89430-c843-4566-bfae-98bdee381c92
📒 Files selected for processing (3)
pytest/modules/Midi/test_midi_creator.pysrc/UltraSinger.pysrc/modules/Midi/midi_creator.py
1. Change concentration_pct comparison from >= to > to align with
docstring ("more than concentration_pct"). Exactly 80% no longer
triggers the shift — only values strictly above 80%.
2. Guard correct_vocal_center() behind settings.vocal_center_correction
flag (default: True, disable with --disable_vocal_center). This
prevents unexpected transpositions for songs with legitimately low
vocal ranges.
3. Update tests: split exact-80% test into two cases (exactly 80%
does NOT trigger, 90% DOES trigger).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai check |
|
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
CodeRabbit noted the test suite only covered the shift path. Add two tests confirming that notes already inside the expected centre band (MIDI 55-79) are never shifted — one near the low end and one near the high end. Note: The other two review comments were false positives: - The concentration check already uses > (not >=) - The call is already behind settings.vocal_center_correction flag Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Resolve conflicts with feat/vocal-center-octave (PR #29): - Settings.py: keep both onset_correction and vocal_center_correction - UltraSinger.py: keep both --disable_vocal_center and --disable_onset_correction CLI options Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Resolve conflicts: keep onset_correction setting and CLI option alongside vocal_center_correction from PR #29. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Add onset-based timing correction for note start times After WhisperX forced alignment, note start times can be offset from actual vocal onsets by up to ~100ms. This adds a post-processing step that detects audio onsets using librosa and snaps nearby note starts to improve timing accuracy. - New module: onset_correction.py with detect_vocal_onsets() and snap_to_onsets() (max 80ms snap distance, preserves min 10ms duration) - Enabled by default, disable with --disable_onset_correction - Runs after transcription, before syllable splitting - Uses whisper_audio_path (un-muted vocals) for onset detection Includes 13 unit tests for edge cases, boundary conditions, multi-note independence, word/timing preservation, and custom parameters. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Address CodeRabbit review: overlap prevention + fail-open - Prevent snap_to_onsets() from moving a note before the previous note's end time by clamping the candidate to max(nearest, prev_end). - Wrap onset correction in UltraSinger.run() in try/except so I/O or decoder errors degrade gracefully instead of aborting the pipeline. - Add two overlap-prevention regression tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Merge main into feat/onset-timing-correction Resolve conflicts with feat/vocal-center-octave (PR #29): - Settings.py: keep both onset_correction and vocal_center_correction - UltraSinger.py: keep both --disable_vocal_center and --disable_onset_correction CLI options Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Anthony <anthony@baseattack.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* Add vocal-centre safety-net for consistent wrong-octave detection
When ALL detected pitches are consistently in the wrong octave (e.g. -12 HT),
correct_global_octave() may not catch it because the median still falls inside
the broad [48,84] range, and correct_octave_outliers() can't help because
there are no outliers to compare against (100% consistency = "correct").
The new correct_vocal_center() function runs AFTER both existing corrections
as a final safety net. It uses a tighter expected centre band (MIDI 55-79,
roughly G3-G5) and checks if >80% of notes cluster below/above the threshold.
If so, it shifts all notes by ±12 semitones.
Pipeline order: global_octave → octave_outliers → vocal_center → manual_shift
Includes 13 unit tests covering edge cases, boundary conditions, concentration
thresholds, interval preservation, and custom parameters.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Fix CodeRabbit findings: strict > comparator and configurable flag
1. Change concentration_pct comparison from >= to > to align with
docstring ("more than concentration_pct"). Exactly 80% no longer
triggers the shift — only values strictly above 80%.
2. Guard correct_vocal_center() behind settings.vocal_center_correction
flag (default: True, disable with --disable_vocal_center). This
prevents unexpected transpositions for songs with legitimately low
vocal ranges.
3. Update tests: split exact-80% test into two cases (exactly 80%
does NOT trigger, 90% DOES trigger).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Add no-shift regression tests for valid centre-band input
CodeRabbit noted the test suite only covered the shift path. Add two
tests confirming that notes already inside the expected centre band
(MIDI 55-79) are never shifted — one near the low end and one near the
high end.
Note: The other two review comments were false positives:
- The concentration check already uses > (not >=)
- The call is already behind settings.vocal_center_correction flag
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Anthony <anthony@baseattack.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* Add onset-based timing correction for note start times After WhisperX forced alignment, note start times can be offset from actual vocal onsets by up to ~100ms. This adds a post-processing step that detects audio onsets using librosa and snaps nearby note starts to improve timing accuracy. - New module: onset_correction.py with detect_vocal_onsets() and snap_to_onsets() (max 80ms snap distance, preserves min 10ms duration) - Enabled by default, disable with --disable_onset_correction - Runs after transcription, before syllable splitting - Uses whisper_audio_path (un-muted vocals) for onset detection Includes 13 unit tests for edge cases, boundary conditions, multi-note independence, word/timing preservation, and custom parameters. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Address CodeRabbit review: overlap prevention + fail-open - Prevent snap_to_onsets() from moving a note before the previous note's end time by clamping the candidate to max(nearest, prev_end). - Wrap onset correction in UltraSinger.run() in try/except so I/O or decoder errors degrade gracefully instead of aborting the pipeline. - Add two overlap-prevention regression tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Merge main into feat/onset-timing-correction Resolve conflicts with feat/vocal-center-octave (PR #29): - Settings.py: keep both onset_correction and vocal_center_correction - UltraSinger.py: keep both --disable_vocal_center and --disable_onset_correction CLI options Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Anthony <anthony@baseattack.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
correct_vocal_center()as a safety-net octave correction for cases where ALL detected pitches are consistently in the wrong octave--octaveoverrideProblem
When the pitch detector consistently locks onto the wrong octave (e.g. all notes -12 HT):
correct_global_octave(low=48, high=84)doesn't catch it because the median (e.g. MIDI 54) still falls inside [48,84]correct_octave_outliers()can't help because there are no outliers — 100% of notes are consistentThis was observed in the Phase 5 benchmark where one song scored 0% Pitch ±2 ST despite having 90.8% Interval accuracy (intervals were correct, just the wrong octave).
Solution
New pipeline step
correct_vocal_center():Pipeline order:
global_octave → octave_outliers → vocal_center → manual_shiftTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests