Minimize gaps produced by quantization algorithm #1540
Conversation
jacobtylerwalls
force-pushed
the
quantization-improvements
branch
from
1d2c80d to
98533df
Compare
| @@ -86,7 +86,7 @@ | |||
|
|
|||
| BestQuantizationMatch = namedtuple( | |||
| 'BestQuantizationMatch', | |||
| ['error', 'tick', 'match', 'signedError', 'divisor'] | |||
| ['remainingGap', 'error', 'tick', 'match', 'signedError', 'divisor'] | |||
There was a problem hiding this comment.
okay, but generally add new attributes to a namedtuple at the end so anyone using [0] still gets the same result.
There was a problem hiding this comment.
Yeah. Unfortunately we could no longer use it for sorting (unless there's something clever I'm not thinking of, like layering a sort function on top, but that's what I took the namedtuple to function as.)
There was a problem hiding this comment.
Generally I've used namedtuples because before it was a standard (anonymous) tuple, so people might still be using access by index or destructuring paradigms.
There was a problem hiding this comment.
ah, yes that's fine. You could sort with key=lambda bqm: (bqm[-1], *bqm) but yeah, too much trouble for backwards compatibility on an internal object.
I did though change it to use min -- there's no need to sort the whole thing if you're just getting the minimum
There was a problem hiding this comment.
ah! That was old code back when the m21 team didn't know its own algorithmic complexity
music21/stream/tests.py
Outdated
| def testInsertIgnoreSort(self): | ||
| '''A sorted stream does not become unsorted when ignoreSort=True.''' | ||
| s = Stream() | ||
| s.repeatAppend(note.Note(), 4) | ||
| s.insert(1, tempo.MetronomeMark(), ignoreSort=True) | ||
| self.assertTrue(s.isSorted) |
There was a problem hiding this comment.
this seems strange to me -- can you explain this? The Stream is no longer sorted -- why is it reporting True for isSorted? I don't think I like this. isSorted should be something that should be able to be trusted unless private methods are being used.
There was a problem hiding this comment.
Ah, I think I just botched the setup of the test case. Inserting something at the end would make a more clear test case, I'll fix it.
What motivated this: the docs for insert say:
If
ignoreSortis True then the inserting does not
change whether the Stream is sorted or not (much faster if you're
going to be inserting dozens
of items that don't change the sort status)
But this test fails on master, which is to say, providing ignoreSort=True does change the sort status, even if the stream stays sorted (as I'll update the test to actually demonstrate!) This is because insert() was unconditionally calling coreElementsChanged with clearIsSorted=True.
isSorted should be something that should be able to be trusted unless private methods are being used.
Yeah, maybe ignoreSort needs to become exposed on coreInsert only?
There was a problem hiding this comment.
How I ended up here was:
- I noticed that MIDI files have a tendency to produce simultaneities that don't collapse into chords, because of differing durations. So iterating elements and checking the element at i + 1 might still give you the same offset; I need the next distinct offset. I didn't want to have to look backwards in the elements array, so I realized I wanted sorted streams.
- So I made MIDI parsing produce a stream with isSorted reported as True.
- Then I went to add a test case to testQuantize, which builds streams with insert(). I then had to debug why the stream was reporting itself as unsorted. I chose between using ignoreSort=True versus doing an explicit sort.
- ignoreSort=True wasn't preserving the value of isSorted (I agree that feels like a should-be-core thing, though)
There was a problem hiding this comment.
Yeah, maybe ignoreSort needs to become exposed on coreInsert only?
yeah, I think that's a much better place. And note that coreInsert still computes and returns whether the stream is guaranteed to be sorted.
But we shouldn't expose anything to the users where they could end up with a stream that needs manual sorting unless they set isSorted = False. Feel free to remove. Or just ignore the test and remove calls from this PR and I'll remove ignoreSort in insert later.
There was a problem hiding this comment.
Reverted changes and just went with an explicit sort 👍 . I think I misdiagnosed exactly what was unsorted in the first place anyway.
The keyword will be removed instead.
|
Ah -- I never did a re-review. Is this set for reviewing again? |
|
Yep! |
| # Must be sorted for quantizing to work optimally. | ||
| s.sort() |
There was a problem hiding this comment.
tiny bit confused by this addition -- Streams are autosorted by default. This seems to be a problem of quantize if it somehow bypasses the normal "sort before iterate/access" paradigm.
There was a problem hiding this comment.
Might have been an artifact from an earlier commit. Does look unnecessary now. 😅
Fixes #1536 by improving the algorithm that "looks ahead" to minimize gaps when quantizing durations from its first draft in #1062, as long as the stream is sorted first.
Looking ahead was the right idea, but using the same divisor was not right, because in a passage of triplets, on beats, you have a tie between divisors (for calculating offsets). For the screenshotted example below, that means the last triplet-eighths of a beat were quantized as sixteenths (because there was a tie for the "best divisor" for the quantization of the offset at the X.0 beat).
This refactor solves the reported issue and also includes a simpler way of achieving #641's requirement of nonzero durations on non-grace notes.
Before
After
