Vibrato-aware pitch-change detection, enable by default - #65
Conversation
…default Replace frame-to-frame pitch comparison with region-median comparison in all three pitch-change detection paths (pitch_change_splitter, pitch_based_note_generator, reference_lyrics_aligner). Vibrato oscillates symmetrically around a centre pitch, so comparing each frame against the region median filters it out naturally without false splits. Also widen the median smoothing filter from 5 to 9 frames to cover roughly one half-cycle of typical vocal vibrato (4-8 Hz at ~16 ms frame intervals). Enable --pitch_change_split by default (was opt-in) — the vibrato-aware detection makes it safe for all vocal styles. Add --no_pitch_change_split to allow disabling it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 54 seconds. ⌛ 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)
📝 WalkthroughWalkthroughEnabled pitch-change splitting by default, made detection vibrato-aware using region-median smoothing (larger median window and forward-sustain validation), added Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI
participant UltraSinger
participant Settings as "Settings\n(config)"
participant Pitcher as "Pitcher\n(pitch_change_splitter)"
participant Output as "Note Segments"
User->>CLI: invoke with/without flags
CLI->>UltraSinger: parse args
UltraSinger->>Settings: set pitch_change_split (True/False)
UltraSinger->>Pitcher: run segmentation (median_filter_window)
Pitcher->>Pitcher: median-smooth -> region-median detection -> forward-sustain check
Pitcher->>Output: emit split/merged MidiSegments
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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)
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 review |
✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/modules/Pitcher/pitch_change_splitter.py (1)
92-179:⚠️ Potential issue | 🔴 CriticalThe two-pass candidate scan can miss later real pitch changes.
candidatesis fully populated before Lines 177-178 run, so resettingregion_valuesthere cannot affect later detection. A contour like60 → 64 → 61with a2 STthreshold will confirm the first jump, but the61region never becomes a candidate because it was compared against the original60center during the first pass. Please collapse detection + sustain confirmation into one forward scan, or rebuild candidates after each confirmed region, so the stable-region center actually advances.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Pitcher/pitch_change_splitter.py` around lines 92 - 179, The two-pass logic in _detect_pitch_change_points lets region_values lag behind confirmed changes (candidates is built from the original region), so later real changes can be missed; fix by merging detection and sustain-confirmation into a single forward scan: inside the main for-loop over midi_values, when diff >= min_semitone_change immediately inspect subsequent frames (using times and midi_values) to compute the sustained region and duration (like the existing sustained_values/sustained_until logic), and if duration >= min_duration_s treat this as a confirmed split (append the current index to confirmed/candidates, set region_values = sustained_values and advance the loop index to the end of the sustained region); if the sustained check fails, treat the midi_val as noise and append it to region_values and continue scanning. This keeps region_center up-to-date and avoids missing later changes.
🧹 Nitpick comments (3)
src/modules/Pitcher/pitch_based_note_generator.py (2)
428-435: Keep the smoothing default in one place.
create_midi_segments_from_pitch()now defaults to9, but_median_filter_midi()still defaults to5. Any future/internal caller that relies on the helper default will silently fall back to the old smoothing behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Pitcher/pitch_based_note_generator.py` around lines 428 - 435, The default smoothing window is inconsistent: create_midi_segments_from_pitch sets median_filter_window=9 but the helper _median_filter_midi still defaults to 5; unify them by adding a single source-of-truth (e.g., MEDIAN_FILTER_WINDOW_DEFAULT) and use that constant as the default in both create_midi_segments_from_pitch and _median_filter_midi (or make _median_filter_midi require the window argument and have create_midi_segments_from_pitch pass its median_filter_window), ensuring both symbols (_median_filter_midi and create_midi_segments_from_pitch) reference the same default.
202-226: Don't re-count the sustained run after confirming a split.Once the forward scan accepts a new region,
region_values = sustained_valuesalready includes the frames you just validated, but the outer loop still resumes atLine 228fromi + 1. That reprocessessustained_values[1:], adds avoidable work, and biases the new region median toward the already-scanned prefix.♻️ Suggested cleanup
if diff >= min_semitone_change: # Check if the new pitch is sustained sustained_values: list[float] = [m] sustained_until = times[i] + last_sustained_idx = i for j in range(i + 1, len(times)): if smoothed_midi[j] is None: continue if abs(smoothed_midi[j] - m) < min_semitone_change: sustained_values.append(smoothed_midi[j]) sustained_until = times[j] + last_sustained_idx = j else: break duration = sustained_until - times[i] if duration >= min_duration_s: # Confirmed pitch change — close current segment, start new if i > current_start: segments.append((current_start, i)) current_start = i region_values = sustained_values + i = last_sustained_idx else: # Not sustained — absorb into current region region_values.append(m)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Pitcher/pitch_based_note_generator.py` around lines 202 - 226, The forward scan that validates a sustained run currently assigns region_values = sustained_values but then continues the outer for-loop from i+1, reprocessing frames already included; fix this by advancing the outer loop index to the end of the sustained run when you accept a split: after collecting sustained_values and computing sustained_until (and identifying the loop variable j that ended the run), set the outer-loop position to the last sustained frame (e.g., j-1) so the next outer iteration starts after the sustained run; apply this change in the block that handles duration >= min_duration_s where region_values = sustained_values, using the same symbols smoothed_midi, times, sustained_values, j, current_start, segments, min_semitone_change, and min_duration_s to locate the code.pytest/modules/Pitcher/test_pitch_change_splitter.py (1)
170-194: Assert the post-change pitch, not just the segment count.
len(result) == 2still passes if the split lands in the wrong place or the second segment keeps the original note. Verifying the resulting note here will make this regression test much harder to satisfy accidentally.✅ Tighten the assertion
# Should split into vibrato region + stable E4 region assert len(result) == 2 assert result[0].word == "slide" + assert result[0].note == "C4" + assert result[1].note == "E4" assert result[1].word in ("~", "~ ")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pytest/modules/Pitcher/test_pitch_change_splitter.py` around lines 170 - 194, Add an assertion that the second resulting segment actually corresponds to the E4 pitch: compute the midpoint time of result[1] (from split_notes_at_pitch_changes output) and look up the frequency in the generated pitched data (from _make_pitched_data), convert it to MIDI (librosa.hz_to_midi) and assert it is approximately 64.0 (e.g. within 0.5 semitone); this ensures test_vibrato_then_real_change not only checks count/words but the post-change pitch is E4.
🤖 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/gui/ultrasinger_runner.py`:
- Around line 352-355: The current branch checks
config.get("pitch_change_split") without a default, so missing keys cause adding
"--no_pitch_change_split" and override the new default; change the test to use
config.get("pitch_change_split", True) so legacy/empty configs align with the
new default-enabled behavior, i.e., update the conditional around config and the
args.append calls that add "--pitch_change_split" or "--no_pitch_change_split"
to use config.get("pitch_change_split", True).
---
Outside diff comments:
In `@src/modules/Pitcher/pitch_change_splitter.py`:
- Around line 92-179: The two-pass logic in _detect_pitch_change_points lets
region_values lag behind confirmed changes (candidates is built from the
original region), so later real changes can be missed; fix by merging detection
and sustain-confirmation into a single forward scan: inside the main for-loop
over midi_values, when diff >= min_semitone_change immediately inspect
subsequent frames (using times and midi_values) to compute the sustained region
and duration (like the existing sustained_values/sustained_until logic), and if
duration >= min_duration_s treat this as a confirmed split (append the current
index to confirmed/candidates, set region_values = sustained_values and advance
the loop index to the end of the sustained region); if the sustained check
fails, treat the midi_val as noise and append it to region_values and continue
scanning. This keeps region_center up-to-date and avoids missing later changes.
---
Nitpick comments:
In `@pytest/modules/Pitcher/test_pitch_change_splitter.py`:
- Around line 170-194: Add an assertion that the second resulting segment
actually corresponds to the E4 pitch: compute the midpoint time of result[1]
(from split_notes_at_pitch_changes output) and look up the frequency in the
generated pitched data (from _make_pitched_data), convert it to MIDI
(librosa.hz_to_midi) and assert it is approximately 64.0 (e.g. within 0.5
semitone); this ensures test_vibrato_then_real_change not only checks
count/words but the post-change pitch is E4.
In `@src/modules/Pitcher/pitch_based_note_generator.py`:
- Around line 428-435: The default smoothing window is inconsistent:
create_midi_segments_from_pitch sets median_filter_window=9 but the helper
_median_filter_midi still defaults to 5; unify them by adding a single
source-of-truth (e.g., MEDIAN_FILTER_WINDOW_DEFAULT) and use that constant as
the default in both create_midi_segments_from_pitch and _median_filter_midi (or
make _median_filter_midi require the window argument and have
create_midi_segments_from_pitch pass its median_filter_window), ensuring both
symbols (_median_filter_midi and create_midi_segments_from_pitch) reference the
same default.
- Around line 202-226: The forward scan that validates a sustained run currently
assigns region_values = sustained_values but then continues the outer for-loop
from i+1, reprocessing frames already included; fix this by advancing the outer
loop index to the end of the sustained run when you accept a split: after
collecting sustained_values and computing sustained_until (and identifying the
loop variable j that ended the run), set the outer-loop position to the last
sustained frame (e.g., j-1) so the next outer iteration starts after the
sustained run; apply this change in the block that handles duration >=
min_duration_s where region_values = sustained_values, using the same symbols
smoothed_midi, times, sustained_values, j, current_start, segments,
min_semitone_change, and min_duration_s to locate the code.
🪄 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: f51bf5f6-4f38-4e94-8f3a-a0b6f88ed0f1
📒 Files selected for processing (11)
README.mdpytest/modules/Pitcher/test_pitch_change_splitter.pysrc/Settings.pysrc/UltraSinger.pysrc/gui/config.pysrc/gui/settings_tab.pysrc/gui/ultrasinger_runner.pysrc/modules/Pitcher/pitch_based_note_generator.pysrc/modules/Pitcher/pitch_change_splitter.pysrc/modules/Speech_Recognition/reference_lyrics_aligner.pysrc/modules/common_print.py
…ertions
- Replace two-pass candidate detection with single-pass forward scan
in pitch_change_splitter to keep region_values current
- Increase median filter window 5→9 in pitch_based_note_generator
(consistent with other modules, covers half vibrato cycle)
- Skip past sustained regions in pitch_based_note_generator to avoid
reprocessing frames
- Add pitch value assertions to test_vibrato_then_real_change
- Add default=True to config.get("pitch_change_split") in GUI runner
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/modules/Pitcher/pitch_change_splitter.py (1)
126-127: Thread the median window through this path too.
src/modules/Pitcher/pitch_based_note_generator.py:433-500already carriesmedian_filter_windowfrom its public API down to the smoothing call. Hard-coding9here means the vibrato-aware paths can drift again on the next retune. I'd pass the window into_detect_pitch_change_points()/split_notes_at_pitch_changes()instead of inlining it here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Pitcher/pitch_change_splitter.py` around lines 126 - 127, Currently the median window is hard-coded as 9 in pitch_change_splitter.py when computing midi_values via _median_smooth; instead thread the existing median_filter_window parameter from the public API down into _detect_pitch_change_points and/or split_notes_at_pitch_changes so the same window is used everywhere: add a median_filter_window parameter to _detect_pitch_change_points and split_notes_at_pitch_changes (and any callers), remove the literal 9, and call _median_smooth(midi_raw, window=median_filter_window) so midi_values respects the passed-in median_filter_window used by pitch_based_note_generator.
🤖 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/Pitcher/pitch_change_splitter.py`:
- Around line 155-166: The loop currently skips None (unvoiced) frames but still
measures sustain from times[i], allowing long unvoiced gaps to count as
sustained; update the inner loop in pitch_change_splitter.py (the for j in
range(...) that references midi_values, times, min_semitone_change,
sustained_until, last_sustained) to treat None as a break condition (i.e., if
midi_values[j] is None: break) so unvoiced frames terminate sustain, or
alternatively compute duration from the last voiced timestamp (use
last_sustained/time_of_last_voiced instead of times[i]) to exclude skipped gaps
from the sustain timer; implement the preferred fix consistently where duration
is computed (duration = sustained_until - <voiced_start> and comparison to
min_duration_s).
---
Nitpick comments:
In `@src/modules/Pitcher/pitch_change_splitter.py`:
- Around line 126-127: Currently the median window is hard-coded as 9 in
pitch_change_splitter.py when computing midi_values via _median_smooth; instead
thread the existing median_filter_window parameter from the public API down into
_detect_pitch_change_points and/or split_notes_at_pitch_changes so the same
window is used everywhere: add a median_filter_window parameter to
_detect_pitch_change_points and split_notes_at_pitch_changes (and any callers),
remove the literal 9, and call _median_smooth(midi_raw,
window=median_filter_window) so midi_values respects the passed-in
median_filter_window used by pitch_based_note_generator.
🪄 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: 05a9bb23-e816-4ad3-8d42-b7bd42180c85
📒 Files selected for processing (4)
pytest/modules/Pitcher/test_pitch_change_splitter.pysrc/gui/ultrasinger_runner.pysrc/modules/Pitcher/pitch_based_note_generator.pysrc/modules/Pitcher/pitch_change_splitter.py
✅ Files skipped from review due to trivial changes (1)
- pytest/modules/Pitcher/test_pitch_change_splitter.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/gui/ultrasinger_runner.py
- src/modules/Pitcher/pitch_based_note_generator.py
…, timing fix - Thread median_filter_window parameter through public API instead of hard-coding 9 in _detect_pitch_change_points - Treat unvoiced (None) frames as sustain break in inner loop to prevent long unvoiced gaps from inflating sustain duration Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/modules/Pitcher/pitch_change_splitter.py (1)
379-395: Document or guard themin_note_duration_ms/median_filter_windowinteraction.Because the median filter runs before the duration check, a wide window can smooth out short pitch regions before they ever reach
min_note_duration_ms. With both knobs public now, callers can pick combinations where the advertised minimum duration is not actually achievable. Consider validating them together against the frame hop or deriving one from the other.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Pitcher/pitch_change_splitter.py` around lines 379 - 395, The median_filter_window is applied before the min_note_duration_ms check which lets a large median window erase short pitch regions so callers can set incompatible values; inside the pitch-change splitting function (the function taking parameters min_note_duration_ms and median_filter_window in pitch_change_splitter.py) add a guard/validation that compares median_filter_window against the frame hop duration (e.g., frame_hop_ms or hop_ms used for pitch frames) and min_note_duration_ms: either compute/derive an effective minimum note duration as median_filter_window * frame_hop_ms and ensure min_note_duration_ms >= that (or raise ValueError), or automatically set min_note_duration_ms = max(min_note_duration_ms, median_filter_window * frame_hop_ms); also update the function docstring to explain the relationship and the enforced constraint so callers know the interaction.
🤖 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/Pitcher/pitch_change_splitter.py`:
- Around line 66-89: The median-smoothing routine currently accepts invalid
window sizes (even, non-positive, non-integer) which silently change behavior;
update _median_smooth to validate its window parameter: ensure window is an int,
>0, and odd (e.g. window % 2 == 1), and raise a ValueError with a clear message
if not; do this validation at the start of the _median_smooth function so
callers (and the public API that threads median_filter_window through) fail fast
instead of silently widening or disabling smoothing.
---
Nitpick comments:
In `@src/modules/Pitcher/pitch_change_splitter.py`:
- Around line 379-395: The median_filter_window is applied before the
min_note_duration_ms check which lets a large median window erase short pitch
regions so callers can set incompatible values; inside the pitch-change
splitting function (the function taking parameters min_note_duration_ms and
median_filter_window in pitch_change_splitter.py) add a guard/validation that
compares median_filter_window against the frame hop duration (e.g., frame_hop_ms
or hop_ms used for pitch frames) and min_note_duration_ms: either compute/derive
an effective minimum note duration as median_filter_window * frame_hop_ms and
ensure min_note_duration_ms >= that (or raise ValueError), or automatically set
min_note_duration_ms = max(min_note_duration_ms, median_filter_window *
frame_hop_ms); also update the function docstring to explain the relationship
and the enforced constraint so callers know the interaction.
🪄 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: 78d8b8d7-85c6-4879-a471-7453c9b4029e
📒 Files selected for processing (1)
src/modules/Pitcher/pitch_change_splitter.py
- _median_smooth now rejects non-positive/non-integer window values and rounds even values up to next odd - Docstring on split_notes_at_pitch_changes documents how median_filter_window interacts with min_note_duration_ms Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/modules/Pitcher/pitch_change_splitter.py (1)
74-81: Make themedian_filter_windowdocs match the runtime behavior.The implementation accepts even values and rounds them up, but both docstrings still say callers must pass an odd integer. The sizing note also reads as if the shipped
9/80 msdefaults are invalid at typical ~16 ms hops. Please document the coercion explicitly and tighten the rule-of-thumb accordingly.Also applies to: 402-407
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Pitcher/pitch_change_splitter.py` around lines 74 - 81, The docstring for median_filter_window currently says callers must pass an odd integer but the function accepts any positive int and coerces even values to the next odd; update the docstring(s) (including the other occurrence around lines 402–407) to state that the parameter accepts any positive integer and that even values will be rounded up to the next odd, and adjust the sizing note to describe the rule-of-thumb in terms of frames/hop (e.g., window length ≈ desired_ms / hop_ms, default window=9 is valid and corresponds to ~9 * hop_ms) so callers understand how the default and recommended sizes relate to typical ~16ms hops. Ensure you reference median_filter_window behavior in the docstring text and mention the coercion explicitly.
🤖 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/Pitcher/pitch_change_splitter.py`:
- Around line 159-175: The sustain check currently compares each subsequent
frame to the initial deviating frame (midi_val) which rejects cases like
60→62→64,64,64; change the loop to validate sustain against a candidate-region
center instead: initialize a running center (e.g., center = midi_val), and for
each j check abs(midi_values[j] - center) < min_semitone_change; when you accept
a frame, append it to sustained_values and recompute the center (mean or median
of sustained_values) before continuing; keep the rest of the logic that sets
sustained_until, last_sustained, duration and the min_duration_s comparison
intact so the decision uses the region-centered sustain test.
---
Nitpick comments:
In `@src/modules/Pitcher/pitch_change_splitter.py`:
- Around line 74-81: The docstring for median_filter_window currently says
callers must pass an odd integer but the function accepts any positive int and
coerces even values to the next odd; update the docstring(s) (including the
other occurrence around lines 402–407) to state that the parameter accepts any
positive integer and that even values will be rounded up to the next odd, and
adjust the sizing note to describe the rule-of-thumb in terms of frames/hop
(e.g., window length ≈ desired_ms / hop_ms, default window=9 is valid and
corresponds to ~9 * hop_ms) so callers understand how the default and
recommended sizes relate to typical ~16ms hops. Ensure you reference
median_filter_window behavior in the docstring text and mention the coercion
explicitly.
🪄 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: 02c8980d-0260-475c-9b88-421219632a0c
📒 Files selected for processing (1)
src/modules/Pitcher/pitch_change_splitter.py
- Sustain check now compares against running candidate-center median instead of the initial deviating frame, accepting gradual slides - Docstrings updated: even window values are coerced (not rejected), rule-of-thumb expressed in frames * hop_ms Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
* Vibrato-aware pitch-change detection, enable --pitch_change_split by default
Replace frame-to-frame pitch comparison with region-median comparison in
all three pitch-change detection paths (pitch_change_splitter,
pitch_based_note_generator, reference_lyrics_aligner). Vibrato oscillates
symmetrically around a centre pitch, so comparing each frame against the
region median filters it out naturally without false splits.
Also widen the median smoothing filter from 5 to 9 frames to cover roughly
one half-cycle of typical vocal vibrato (4-8 Hz at ~16 ms frame intervals).
Enable --pitch_change_split by default (was opt-in) — the vibrato-aware
detection makes it safe for all vocal styles. Add --no_pitch_change_split
to allow disabling it.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Address CodeRabbit review: single-pass scan, vibrato window, test assertions
- Replace two-pass candidate detection with single-pass forward scan
in pitch_change_splitter to keep region_values current
- Increase median filter window 5→9 in pitch_based_note_generator
(consistent with other modules, covers half vibrato cycle)
- Skip past sustained regions in pitch_based_note_generator to avoid
reprocessing frames
- Add pitch value assertions to test_vibrato_then_real_change
- Add default=True to config.get("pitch_change_split") in GUI runner
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Address CodeRabbit review: conf-threshold threading, 0-D array guards, timing fix
- Thread median_filter_window parameter through public API instead of
hard-coding 9 in _detect_pitch_change_points
- Treat unvoiced (None) frames as sustain break in inner loop to prevent
long unvoiced gaps from inflating sustain duration
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Validate median window param, document filter/duration interaction
- _median_smooth now rejects non-positive/non-integer window values
and rounds even values up to next odd
- Docstring on split_notes_at_pitch_changes documents how
median_filter_window interacts with min_note_duration_ms
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Address CodeRabbit review: region-centered sustain, docstring accuracy
- Sustain check now compares against running candidate-center median
instead of the initial deviating frame, accepting gradual slides
- Docstrings updated: even window values are coerced (not rejected),
rule-of-thumb expressed in frames * hop_ms
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>
1 participant
Summary
--pitch_change_splitis now on by default. The vibrato-aware detection makes it safe for all vocal styles. Add--no_pitch_change_splitto disable.Affected modules
pitch_change_splitter.py_median_smooth()utilitypitch_based_note_generator.py_segment_voiced_region(), median window 5→9reference_lyrics_aligner.pySettings.pypitch_change_splitdefaultFalse→True--no_pitch_change_splitflag, updated help text, GUI tooltip, config defaultsTest plan
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests