Skip to content
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.

Sign up for GitHub

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

Minimize gaps produced by quantization algorithm #1540

Merged
merged 13 commits into from
May 1, 2023
Merged

Minimize gaps produced by quantization algorithm #1540

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
98533df
Minimize gaps produced by quantization algorithm
jacobtylerwalls Mar 25, 2023
f64ea43
fixup! Minimize
jacobtylerwalls Mar 25, 2023
517f046
fixup! Minimize
jacobtylerwalls Mar 25, 2023
fe5186a
fixup! Minimize
jacobtylerwalls Mar 26, 2023
d4ebe55
Add regression test for insert(ignoreSort=True)
jacobtylerwalls Mar 26, 2023
a7135d3
fixup! Add regression test
jacobtylerwalls Mar 26, 2023
1ea8840
fixup! Minimize
jacobtylerwalls Mar 26, 2023
44450c4
Revert changes to insert() in favor of explicit sort in test
jacobtylerwalls Apr 1, 2023
65692c8
Remove test relating to insert(ignoreSort=True)
jacobtylerwalls Apr 1, 2023
5c232c9
Merge branch 'master' into quantization-improvements
mscuthbert May 1, 2023
0206700
No need to sort all if just getting the minimum
mscuthbert May 1, 2023
18b750f
rem unused heapq
mscuthbert May 1, 2023
df97c67
trailing whitespace (editing on github, bad idea)
mscuthbert May 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion music21/_version.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
'''
from __future__ import annotations

__version__ = '9.0.0a9'
__version__ = '9.0.0a10'


def get_version_tuple(vv):
Expand Down
2 changes: 1 addition & 1 deletion music21/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
<class 'music21.base.Music21Object'>

>>> music21.VERSION_STR
'9.0.0a9'
'9.0.0a10'

Alternatively, after doing a complete import, these classes are available
under the module "base":
Expand Down
2 changes: 1 addition & 1 deletion music21/midi/translate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2065,7 +2065,7 @@ def midiTrackToStream(
singleN.editorial.midiTickStart = notes[0][0][0]
s.coreInsert(o, singleN)

s.coreElementsChanged()
s.sort(force=True) # will also run coreElementsChanged()
# quantize to nearest 16th
if quantizePost:
s.quantize(quarterLengthDivisors=quarterLengthDivisors,
Expand Down
80 changes: 48 additions & 32 deletions music21/stream/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@

BestQuantizationMatch = namedtuple(
'BestQuantizationMatch',
['error', 'tick', 'match', 'signedError', 'divisor']
['remainingGap', 'error', 'tick', 'match', 'signedError', 'divisor']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, but generally add new attributes to a namedtuple at the end so anyone using [0] still gets the same result.

Copy link
Member Author

@jacobtylerwalls jacobtylerwalls Mar 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah! That was old code back when the m21 team didn't know its own algorithmic complexity

)

class StreamDeprecationWarning(UserWarning):
Expand Down Expand Up @@ -2344,7 +2344,7 @@ def insert(self,
updateIsFlat = False
if element.isStream:
updateIsFlat = True
self.coreElementsChanged(updateIsFlat=updateIsFlat)
self.coreElementsChanged(updateIsFlat=updateIsFlat, clearIsSorted=not ignoreSort)
if ignoreSort is False:
self.isSorted = storeSorted

Expand Down Expand Up @@ -9361,18 +9361,37 @@ def quantize(
# this presently is not trying to avoid overlaps that
# result from quantization; this may be necessary

def bestMatch(target, divisors):
def bestMatch(target, divisors, zeroAllowed=True, gapToFill=0.0):
found = []
for div in divisors:
match, error, signedErrorInner = common.nearestMultiple(target, (1 / div))
# Sort by unsigned error, then "tick" (divisor expressed as QL, e.g. 0.25)
found.append(BestQuantizationMatch(error, 1 / div, match, signedErrorInner, div))
tick = 1 / div # divisor expressed as QL, e.g. 0.25
match, error, signedErrorInner = common.nearestMultiple(target, tick)
if not zeroAllowed and match == 0.0:
match = tick
signedErrorInner = round(target - match, 7)
error = abs(signedErrorInner)
if gapToFill % tick == 0:
remainingGap = 0.0
else:
remainingGap = max(gapToFill - match, 0.0)
# Sort by remainingGap, then unsigned error, then tick
found.append(
BestQuantizationMatch(
remainingGap, error, tick, match, signedErrorInner, div))
# get first, and leave out the error
bestMatchTuple = sorted(found)[0]
return bestMatchTuple

# if we have a min of 0.25 (sixteenth)
# quarterLengthMin = quarterLengthDivisors[0]
def findNextElementNotCoincident(
useStream: Stream,
startIndex: int,
) -> tuple[base.Music21Object | None, BestQuantizationMatch | None]:
for next_el in useStream._elements[startIndex:]:
next_offset = useStream.elementOffset(next_el)
look_ahead_result = bestMatch(float(next_offset), quarterLengthDivisors)
if look_ahead_result.match > o:
return next_el, look_ahead_result
return None, None

if inPlace is False:
returnStream = self.coreCopyAsDerivation('quantize')
Expand All @@ -9385,6 +9404,11 @@ def bestMatch(target, divisors):

rests_lacking_durations: list[note.Rest] = []
for useStream in useStreams:
# coreSetElementOffset() will immediately set isSorted = False,
# but we need to know if the stream was originally sorted to know
# if it's worth "looking ahead" to the next offset. If a stream
# is unsorted originally, this "looking ahead" could become O(n^2).
originallySorted = useStream.isSorted
for i, e in enumerate(useStream._elements):
if processOffsets:
o = useStream.elementOffset(e)
Expand All @@ -9393,35 +9417,27 @@ def bestMatch(target, divisors):
sign = -1
o = -1 * o
o_matchTuple = bestMatch(float(o), quarterLengthDivisors)
useStream.coreSetElementOffset(e, o_matchTuple.match * sign)
o = o_matchTuple.match * sign
useStream.coreSetElementOffset(e, o)
if hasattr(e, 'editorial') and o_matchTuple.signedError != 0:
e.editorial.offsetQuantizationError = o_matchTuple.signedError * sign
if processDurations:
ql = e.duration.quarterLength
ql = max(ql, 0) # negative ql possible in buggy MIDI files?
d_matchTuple = bestMatch(float(ql), quarterLengthDivisors)
# Check that any gaps from this quantized duration to the next onset
# are at least as large as the smallest quantization unit (the largest divisor)
# If not, then re-quantize this duration with the divisor
# that will be used to quantize the next element's offset
if processOffsets and i + 1 < len(useStream._elements):
next_element = useStream._elements[i + 1]
next_offset = useStream.elementOffset(next_element)
look_ahead_result = bestMatch(float(next_offset), quarterLengthDivisors)
next_offset = look_ahead_result.match
next_divisor = look_ahead_result.divisor
if (0 < next_offset - (e.offset + d_matchTuple.match)
< 1 / max(quarterLengthDivisors)):
# Overwrite the earlier matchTuple with a better result
d_matchTuple = bestMatch(float(ql), (next_divisor,))
# Enforce nonzero duration for non-grace notes
if (d_matchTuple.match == 0
and isinstance(e, note.NotRest)
and not e.duration.isGrace):
e.quarterLength = 1 / max(quarterLengthDivisors)
if hasattr(e, 'editorial'):
e.editorial.quarterLengthQuantizationError = ql - e.quarterLength
elif d_matchTuple.match == 0 and isinstance(e, note.Rest):
zeroAllowed = not isinstance(e, note.NotRest) or e.duration.isGrace
if processOffsets and originallySorted:
next_element, look_ahead_result = (
findNextElementNotCoincident(useStream, i + 1)
)
if next_element is not None and look_ahead_result is not None:
gapToFill = opFrac(look_ahead_result.match - e.offset)
d_matchTuple = bestMatch(
float(ql), quarterLengthDivisors, zeroAllowed, gapToFill)
else:
d_matchTuple = bestMatch(float(ql), quarterLengthDivisors, zeroAllowed)
else:
d_matchTuple = bestMatch(float(ql), quarterLengthDivisors, zeroAllowed)
if d_matchTuple.match == 0 and isinstance(e, note.Rest):
rests_lacking_durations.append(e)
else:
e.duration.quarterLength = d_matchTuple.match
Expand Down
21 changes: 20 additions & 1 deletion music21/stream/tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4026,7 +4026,7 @@ def procCompare(srcOffset, srcDur, dstOffset, dstDur, divList):
for i in range(len(srcDur)):
n = note.Note()
n.quarterLength = srcDur[i]
s.insert(srcOffset[i], n)
s.insert(srcOffset[i], n, ignoreSort=True)

s.quantize(divList, processOffsets=True, processDurations=True, inPlace=True)

Expand Down Expand Up @@ -4071,6 +4071,18 @@ def procCompare(srcOffset, srcDur, dstOffset, dstDur, divList):

[8, 6]) # snap to 0.125 and 0.1666666

# User-reported example: contains overlap and tiny gaps
# Parsing with fewer gaps in v.9, as long as stream is sorted
# https://github.com/cuthbertLab/music21/issues/1536
procCompare([2.016, 2.026, 2.333, 2.646, 3.0, 3.323, 3.651],
[0.123, 0.656, 0.104, 0.094, 0.146, 0.099, 0.141],

[2, 2, F('7/3'), F('8/3'), 3.0, F('10/3'), F('11/3')],
[F('1/3'), F('2/3'), F('1/3'), F('1/3'),
F('1/3'), F('1/3'), 0.25],

[4, 3])

def testQuantizeMinimumDuration(self):
'''
Notes (not rests!) of nonzero duration should retain a nonzero
Expand Down Expand Up @@ -4726,6 +4738,13 @@ def testSortAndAutoSort(self):
self.assertEqual([(0.0, 2), (0.0, 30), (5.0, 25), (8.0, 10), (10.0, 2),
(15.0, 10), (20.0, 2), (22.0, 1.0)], match)

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ignoreSort is 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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How I ended up here was:

  1. 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.
  2. So I made MIDI parsing produce a stream with isSorted reported as True.
  3. 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.
  4. ignoreSort=True wasn't preserving the value of isSorted (I agree that feels like a should-be-core thing, though)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted changes and just went with an explicit sort 👍 . I think I misdiagnosed exactly what was unsorted in the first place anyway.


def testMakeChordsBuiltA(self):
# test with equal durations
pitchCol = [('C2', 'A2'),
Expand Down