Repository navigation
fix: don't abort the run on zero/negative-length midi segments #326
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| """Tests for the midi_creator.py module. | ||
|
|
||
| Regression tests for degenerate MidiSegments: UltraSinger's syllable splitting and | ||
| segment merging can produce segments whose `end` is equal to or earlier than their | ||
| `start`. Such a segment used to crash the whole run when writing the MIDI file: | ||
|
|
||
| ValueError: Note end time must be greater than start time | ||
| (pretty_midi/containers.py, Note.__init__) | ||
|
|
||
| Note on pretty_midi's semantics (measured, pretty_midi 0.2.11): | ||
|
|
||
| pretty_midi.Note(100, 60, 3.0, 3.0) -> accepted (end == start) | ||
| pretty_midi.Note(100, 60, 5.0, 4.0) -> ValueError (end < start) | ||
|
|
||
| The crash is therefore only triggered by `end < start`, even though the exception | ||
| text asks for *greater than*. The MIDI writer guards on `end <= start` on purpose: a | ||
| zero-length note carries no information and the same root cause produces invalid | ||
| `duration <= 0` lines in the sibling UltraStar output. The MIDI writer must tolerate | ||
| both instead of aborting the run. | ||
| """ | ||
|
|
||
| import unittest | ||
|
|
||
| from src.modules.Midi.MidiSegment import MidiSegment | ||
| from src.modules.Midi.midi_creator import create_midi_instrument | ||
|
|
||
|
|
||
| class TestCreateMidiInstrument(unittest.TestCase): | ||
|
|
||
| def test_keeps_valid_segments(self): | ||
| # Arrange | ||
| midi_segments = [ | ||
| MidiSegment(note="C4", start=1.0, end=2.0, word="a"), | ||
| MidiSegment(note="D4", start=2.5, end=4.0, word="b"), | ||
| ] | ||
|
|
||
| # Act | ||
| instrument = create_midi_instrument(midi_segments) | ||
|
|
||
| # Assert | ||
| self.assertEqual(2, len(instrument.notes)) | ||
| self.assertEqual([1.0, 2.5], [note.start for note in instrument.notes]) | ||
|
|
||
| def test_skips_zero_length_segment(self): | ||
| # Arrange: end == start. pretty_midi accepts this, but a zero-length note is a | ||
| # no-op; the same root cause also yields invalid `duration 0` UltraStar lines. | ||
| midi_segments = [ | ||
| MidiSegment(note="C4", start=1.0, end=2.0, word="a"), | ||
| MidiSegment(note="C4", start=3.0, end=3.0, word="b"), | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Real solution for all (.txt, .mid .whatever) would be that this is never in the MidiSegment in the first palace. But its good to keep this in the test |
||
| ] | ||
|
|
||
| # Act | ||
| instrument = create_midi_instrument(midi_segments) | ||
|
|
||
| # Assert: the degenerate note is dropped, the valid one survives | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove ai slop comment. When we fix the degeneration in the right place, this comment would lie here. |
||
| self.assertEqual(1, len(instrument.notes)) | ||
| self.assertEqual(1.0, instrument.notes[0].start) | ||
|
|
||
| def test_skips_negative_length_segment(self): | ||
| # Arrange: end < start (seen in real runs as word='~' start=155.25 end=153.78) | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove ai slop comment. We see this state in the code below |
||
| midi_segments = [ | ||
| MidiSegment(note="C4", start=1.0, end=2.0, word="a"), | ||
| MidiSegment(note="E4", start=155.2537142857143, end=153.776, word="~"), | ||
| ] | ||
|
|
||
| # Act | ||
| instrument = create_midi_instrument(midi_segments) | ||
|
|
||
| # Assert | ||
| self.assertEqual(1, len(instrument.notes)) | ||
|
|
||
| def test_all_segments_degenerate_does_not_raise(self): | ||
| # Arrange | ||
| midi_segments = [ | ||
| MidiSegment(note="C4", start=5.0, end=5.0, word="a"), | ||
| MidiSegment(note="C4", start=9.0, end=8.0, word="b"), | ||
| ] | ||
|
|
||
| # Act | ||
| instrument = create_midi_instrument(midi_segments) | ||
|
|
||
| # Assert | ||
| self.assertEqual(0, len(instrument.notes)) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -28,6 +28,15 @@ def create_midi_instrument(midi_segments: list[MidiSegment]) -> object: | |||||||||||||||
| velocity = 100 | ||||||||||||||||
|
|
||||||||||||||||
| for i, midi_segment in enumerate(midi_segments): | ||||||||||||||||
| # Syllable splitting and segment merging can produce segments whose end is equal to | ||||||||||||||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove unessesary ai slop comments |
||||||||||||||||
| # or earlier than their start. pretty_midi rejects a negative length with | ||||||||||||||||
| # "Note end time must be greater than start time", which used to abort the whole run | ||||||||||||||||
| # *after* the UltraStar file had already been written. Skip such segments instead; | ||||||||||||||||
| # their lyric event is still emitted by __create_midi(). | ||||||||||||||||
| if midi_segment.end <= midi_segment.start: | ||||||||||||||||
| print(f"{ULTRASINGER_HEAD} WARNING: skipping degenerate midi segment [{i}] " | ||||||||||||||||
| f"word={midi_segment.word!r} start={midi_segment.start} end={midi_segment.end}") | ||||||||||||||||
| continue | ||||||||||||||||
| note = pretty_midi.Note(velocity, librosa.note_to_midi(midi_segment.note), midi_segment.start, midi_segment.end) | ||||||||||||||||
| instrument.notes.append(note) | ||||||||||||||||
|
|
||||||||||||||||
|
|
@@ -131,6 +140,12 @@ def create_midi_note_from_pitched_data(start_time: float, end_time: float, pitch | |||||||||||||||
| start = find_nearest_index(pitched_data.times, start_time) | ||||||||||||||||
| end = find_nearest_index(pitched_data.times, end_time) | ||||||||||||||||
|
|
||||||||||||||||
| # Surface degenerate segments early - they are the root cause of the guard in | ||||||||||||||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove unessesary ai slop comments |
||||||||||||||||
| # create_midi_instrument(). | ||||||||||||||||
| if end_time <= start_time: | ||||||||||||||||
| print(f"{ULTRASINGER_HEAD} WARNING: degenerate transcript segment word={word!r} " | ||||||||||||||||
| f"start={start_time} end={end_time}") | ||||||||||||||||
|
Comment on lines
+145
to
+147
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win Return before pitch extraction for negative-duration segments. For Return a Proposed fix if end_time <= start_time:
print(f"{ULTRASINGER_HEAD} WARNING: degenerate transcript segment word={word!r} "
f"start={start_time} end={end_time}")
+ return MidiSegment("", start_time, end_time, word)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||
|
|
||||||||||||||||
| if start == end: | ||||||||||||||||
| freqs = [pitched_data.frequencies[start]] | ||||||||||||||||
| confs = [pitched_data.confidence[start]] | ||||||||||||||||
|
|
||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this entire ai slop description.
This description will lie in the future when we fix this issue on the right place and when this test class will be expanded.