Repository: cuthbertLab/music21
Pull request #986 (2021-04-26 15:09:00)

makeRests() gets bar duration context for voices from measures and recurses into Parts and checks for measure padding
Handling some cases not handled by #922.

**Before**
This describes the first bullet point under "Now", below.
#922 was the first effort toward recursing in `makeRests()`. It wasn't quite enough -- final rests in Voices weren't created from the last element to the end of the bar, because the Voice objects don't have a `.barDuration`. Then, a loose rest was eventually created outside of the voices on the Measure object (see the regression test I included--the first test would have failed). Update: I'm actually only describing the MIDI parsing workflow. Reading voices as defined in a musicXML file is unaffected.

**Now**
- refactored `makeRests` to handle voices early enough that measure bar duration is accessible for voices
- recursed over parts, since that was the remaining nested stream container to handle
- defined and documented priority between keyword args `timeRangeFromBarDuration` and `refStreamOrTimeRange`
- made `timeRangeFromBarDuration` account for measure padding
- documented what was a missing feature on `makeVoices()` that now we handle creating trailing rests in voices when an incomplete measure is provided
- rests created by musicxml export AFTER makeNotation runs are now hidden

**Future**

`timeRangeFromBarDuration` should probably default True. 
 Nope! Too presumptuous--don't want to "complete" every incomplete voice parsed from musicXML. 
Some tests fail in interesting ways if I flip the default, suggesting possibly better results, but also that some work might be needed with `.paddingLeft`.
 Handled the padding issues.


coveralls: 
[![Coverage Status](https://coveralls.io/builds/39127357/badge)](https://coveralls.io/builds/39127357)

Coverage increased (+0.002%) to 92.243% when pulling **5eda438edad70f865528d5af612f1916a0a0c2fb on jacobtylerwalls:makeRests-keyword-priority** into **a59c91e4fae7a38fdd935b7e15bad4b74b95b627 on cuthbertLab:master**.

jacobtylerwalls: Ah, the .paddingLeft issues are a regression in #922 because we uncovered functionality and didn't handle this case. makeRests() *is* called in MusicXML export, just didn't do much if you already had measures (again, only streams made from MIDI were typically in that scenario before v7, but now even those have measures).

I'll push another commit to handle paddingLeft
jacobtylerwalls: > I think it looks good -- I don't remember all the subtilties, but if you're confident, feel free to merge. Otherwise I'll take a deep dive.

Thanks for having a look. I think it's time to merge this. Found another rest issue, but one that's piano-specific, can reproduce on v.3.1.0 and doesn't look any worse with this patch, so if you want to don your diving goggles we can do that then. In the meantime we have some output that's wrong we can fix by merging this and bumping the minor version number.
jacobtylerwalls: This is worth a second (shallow) dive given that I added three more commits. Feel good about this now, but again, would appreciate a double-check.
jacobtylerwalls: I'll be merging this shortly. Not trying to rush anything! Just have another PR waiting on this, and in hindsight, the last set of changes doesn't really seem to require another substantive review. Those last changes were:
- Avoiding redundant calls to makeRests given this PR's new recursion by part
- Hiding rests created on musicxml export, since we have decided spacer rests == hidden rests.

I'm squashing the trivial whitespace and typo fixes but leaving substantive commits for the independent pieces in the offchance we need to revert anything later. But this leaves the master branch in a better state to be merging this now, in order to resolve the issue with measure padding not being observed (causing extra rests on export).

Patch:
diff --git a/music21/_version.py b/music21/_version.py
index 333fa7463..57a202038 100644
--- a/music21/_version.py
+++ b/music21/_version.py
@@ -42,7 +42,7 @@ When changing, update the single test case in base.py.
 Changing this number invalidates old pickles -- do it if the old pickles create a problem.
 '''
 
-__version_info__ = (7, 0, 5)  # can be 4-tuple: (7, 0, 5, 'a2')
+__version_info__ = (7, 0, 6)  # can be 4-tuple: (7, 0, 5, 'a2')
 
 v = '.'.join(str(x) for x in __version_info__[0:3])
 if len(__version_info__) > 3 and __version_info__[3]:
diff --git a/music21/base.py b/music21/base.py
index 262e04b42..3a5f13351 100644
--- a/music21/base.py
+++ b/music21/base.py
@@ -28,7 +28,7 @@ available after importing `music21`.
 <class 'music21.base.Music21Object'>
 
 >>> music21.VERSION_STR
-'7.0.5'
+'7.0.6'
 
 Alternatively, after doing a complete import, these classes are available
 under the module "base":
diff --git a/music21/musicxml/m21ToXml.py b/music21/musicxml/m21ToXml.py
index 0301cb43f..eb264f75b 100644
--- a/music21/musicxml/m21ToXml.py
+++ b/music21/musicxml/m21ToXml.py
@@ -1557,18 +1557,34 @@ class ScoreExporter(XMLExporterBase, PartStaffExporterMixin):
         called by .parse() if the score has individual parts.
 
         Calls makeRests() for the part, then creates a PartExporter for each part,
-        and runs .parse() on that part.  appends the PartExporter to self.partExporterList
+        and runs .parse() on that part.  appends the PartExporter to self.
+
+        Hide rests created at this late stage.
+
+        >>> v = stream.Voice(note.Note())
+        >>> m = stream.Measure([meter.TimeSignature(), v])
+        >>> GEX = musicxml.m21ToXml.GeneralObjectExporter(m)
+        >>> out = GEX.parse()  # out is bytes
+        >>> outStr = out.decode('utf-8')  # now is string
+        >>> '<note print-object="no" print-spacing="yes">' in outStr
+        True
         '''
-        # would like to do something like this but cannot
-        # replace object inside of the stream
-        sp = list(self.parts)
-        for innerStream in sp:
-            innerStream.makeRests(self.refStreamOrTimeRange, inPlace=True)
+        # self.parts is a stream of parts
+        # hide any rests created at this late stage, because we are
+        # merely trying to fill up MusicXML display, not impose things on users
+        self.parts.makeRests(refStreamOrTimeRange=self.refStreamOrTimeRange,
+                             inPlace=True,
+                             hideRests=True,
+                             timeRangeFromBarDuration=True,
+                             )
 
         count = 0
+        sp = list(self.parts)
         for innerStream in sp:
             count += 1
-            if count > len(sp):
+            # This guards against making an error in a future refactor
+            # Raises if editing while iterating instead of casting to list above
+            if count > len(sp):  # pragma: no cover
                 raise MusicXMLExportException('infinite stream encountered')
 
             pp = PartExporter(innerStream, parent=self)
@@ -6493,6 +6509,17 @@ class Test(unittest.TestCase):
         rest = tree.find('.//rest')
         self.assertEqual(rest.get('measure'), 'yes')
 
+    def testMeasurePadding(self):
+        from music21 import converter
+        s = stream.Score([converter.parse('tinyNotation: 4/4 c4')])
+        s[stream.Measure].first().paddingLeft = 2.0
+        s[stream.Measure].first().paddingRight = 1.0
+        tree = self.getET(s)
+        self.assertEqual(len(tree.findall('.//rest')), 0)
+        s[stream.Measure].first().paddingLeft = 1.0
+        tree = self.getET(s)
+        self.assertEqual(len(tree.findall('.//rest')), 1)
+
 
 class TestExternal(unittest.TestCase):  # pragma: no cover
 
diff --git a/music21/stream/base.py b/music21/stream/base.py
index c121c48cb..044d7e6a1 100644
--- a/music21/stream/base.py
+++ b/music21/stream/base.py
@@ -10526,6 +10526,9 @@ class Stream(core.StreamCoreMixin, base.Music21Object):
         [<music21.pitch.Pitch C4>]
         >>> [str(n.pitch) for n in s.voices[1].notes]
         ['B-4', 'B-4', 'B-4', 'B-4', 'B-4', 'B-4', 'B-4', 'B-4']
+
+        Changed in v7 -- if `fillGaps=True` and called on an incomplete measure,
+        makes trailing rests in voices. This scenario occurs when parsing MIDI.
         '''
         # this method may not always
         # produce the optimal voice assignment based on context (register
@@ -10575,9 +10578,12 @@ class Stream(core.StreamCoreMixin, base.Music21Object):
         # remove any unused voices (possible if overlap group has sus)
         for v in voices:
             if v:  # skip empty voices
-                if fillGaps:
-                    returnObj.makeRests(fillGaps=True, inPlace=True)
                 returnObj.insert(0, v)
+        if fillGaps:
+            returnObj.makeRests(fillGaps=True,
+                                inPlace=True,
+                                timeRangeFromBarDuration=True,
+                                )
         # remove rests in returnObj
         returnObj.removeByClass('Rest')
         # elements changed will already have been called
diff --git a/music21/stream/makeNotation.py b/music21/stream/makeNotation.py
index 227a6ed05..bded35368 100644
--- a/music21/stream/makeNotation.py
+++ b/music21/stream/makeNotation.py
@@ -711,7 +711,9 @@ def makeRests(
     fill with one Rest preceding this offset.
     This can be called on any Stream,
     a Measure alone, or a Measure that contains
-    Voices.
+    Voices. This method recurses into Parts, Measures, and Voices,
+    since users are unlikely to want "loose" rests outside
+    of sub-containers.
 
     If `refStreamOrTimeRange` is provided as a Stream, this
     Stream is used to get min and max offsets. If a list is provided,
@@ -722,8 +724,15 @@ def makeRests(
     time regions that have no active elements.
 
     If `timeRangeFromBarDuration` is True, and the calling Stream
-    is a Measure with a TimeSignature, the time range will be determined
-    based on the .barDuration property.
+    is a Measure with a TimeSignature (or a Part containing them),
+    the time range will be determined
+    by taking the :meth:`~music21.stream.Measure.barDuration` and subtracting
+    :attr:`~music21.stream.Measure.paddingLeft` and
+    :attr:`~music21.stream.Measure.paddingRight`.
+    This keyword takes priority over `refStreamOrTimeRange`.
+    If both are provided, `timeRangeFromBarDuration`
+    prevails, unless no TimeSignature can be found, in which case, the function
+    falls back to `refStreamOrTimeRange`.
 
     If `inPlace` is True, this is done in-place; if `inPlace` is False,
     this returns a modified deepcopy.
@@ -811,7 +820,11 @@ def makeRests(
 
     Changed in v6 -- all but first attribute are keyword only
 
-    Changed in v7 -- `inPlace` defaults False.
+    Changed in v7:
+
+      - `inPlace` defaults False
+      - Recurses into parts, measures, voices
+      - Gave priority to `timeRangeFromBarDuration` over `refStreamOrTimeRange`
     '''
     from music21 import stream
 
@@ -821,35 +834,57 @@ def makeRests(
     else:
         returnObj = s
 
-    oLowTarget = 0
-    oHighTarget = 0
+    if returnObj.iter.parts:
+        for inner_part in returnObj.iter.parts:
+            inner_part.makeRests(
+                inPlace=True,
+                fillGaps=fillGaps,
+                hideRests=hideRests,
+                refStreamOrTimeRange=refStreamOrTimeRange,
+                timeRangeFromBarDuration=timeRangeFromBarDuration,
+            )
+        return returnObj
 
-    # environLocal.printDebug([
-    #    'makeRests(): object lowestOffset, highestTime', oLow, oHigh])
-    if refStreamOrTimeRange is None:  # use local
-        oLowTarget = 0
-        if timeRangeFromBarDuration and returnObj.isMeasure:
-            # NOTE: this will raise an exception if no meter can be found
-            oHighTarget = returnObj.barDuration.quarterLength
-        elif timeRangeFromBarDuration and returnObj.hasMeasures():
+    def oHighTargetForMeasure(m: stream.Measure) -> float:
+        '''Needed for timeRangeFromBarDuration'''
+        # NOTE: this returns 0.0 if no meter can be found
+        post = m.barDuration.quarterLength
+        post -= m.paddingLeft
+        post -= m.paddingRight
+        return max(post, 0.0)
+
+    oLowTarget = 0.0
+    oHighTarget = 0.0
+    if timeRangeFromBarDuration:
+        if returnObj.isMeasure:
+            oHighTarget = oHighTargetForMeasure(returnObj)
+        elif (
+            stream.Voice in returnObj.classSet
+            and hasattr(refStreamOrTimeRange, 'isMeasure')
+            and refStreamOrTimeRange.isMeasure
+        ):
+            # Alternative to getting measure context would be to access .activeSite
+            # but for now, depend on getting measure context from refStreamOrTimeRange
+            # since we have not documented calling makeRests directly on a Voice and
+            # expecting to infer barDuration from the measure context.
+            # merely trying to support the recursive call for contained voices, below
+            oHighTarget = oHighTargetForMeasure(refStreamOrTimeRange)
+        elif returnObj.hasMeasures():
             oHighTarget = sum(
                 m.barDuration.quarterLength for m in returnObj.getElementsByClass(stream.Measure)
             )
-        else:
+    # If the above search didn't run or still yielded 0.0, use refStreamOrTimeRange
+    if oHighTarget == 0.0:
+        if refStreamOrTimeRange is None:  # use local
             oHighTarget = returnObj.highestTime
-    elif isinstance(refStreamOrTimeRange, stream.Stream):
-        oLowTarget = refStreamOrTimeRange.lowestOffset
-        oHighTarget = refStreamOrTimeRange.highestTime
-        # environLocal.printDebug([
-        #    'refStream used in makeRests', oLowTarget, oHighTarget,
-        #    len(refStreamOrTimeRange)])
-    # treat as a list
-    elif common.isIterable(refStreamOrTimeRange):
-        oLowTarget = min(refStreamOrTimeRange)
-        oHighTarget = max(refStreamOrTimeRange)
-        # environLocal.printDebug([
-        #    'offsets used in makeRests', oLowTarget, oHighTarget,
-        #    len(refStreamOrTimeRange)])
+        elif isinstance(refStreamOrTimeRange, stream.Stream):
+            oLowTarget = refStreamOrTimeRange.lowestOffset
+            oHighTarget = refStreamOrTimeRange.highestTime
+        # treat as a list
+        elif common.isIterable(refStreamOrTimeRange):
+            oLowTarget = min(refStreamOrTimeRange)
+            oHighTarget = max(refStreamOrTimeRange)
+
     if returnObj.hasVoices():
         bundle = list(returnObj.voices)
     elif returnObj.hasMeasures():
@@ -858,12 +893,25 @@ def makeRests(
         bundle = [returnObj]
 
     # bundle components may be voices, measures, or a flat Stream
-    for v in bundle:
-        oLow = v.lowestOffset
-        oHigh = v.highestTime
-        if returnObj.hasMeasures():
+    for component in bundle:
+        oLow = component.lowestOffset
+        oHigh = component.highestTime
+        if component.isMeasure:
+            if timeRangeFromBarDuration:
+                oHighTarget = oHighTargetForMeasure(component)
+            # process voices
+            for inner_voice in component.voices:
+                inner_voice.makeRests(inPlace=True,
+                                      fillGaps=fillGaps,
+                                      hideRests=hideRests,
+                                      refStreamOrTimeRange=component,
+                                      timeRangeFromBarDuration=timeRangeFromBarDuration,
+                                      )
+            # Refresh these variables given that inner voices were altered
+            oLow = component.lowestOffset
+            oHigh = component.highestTime
             # adjust oHigh to not exceed measure
-            oHighTarget = min(v.barDuration.quarterLength, oHighTarget)
+            oHighTarget = min(component.barDuration.quarterLength, oHighTarget)
 
         # create rest from start to end
         qLen = oLow - oLowTarget
@@ -873,27 +921,25 @@ def makeRests(
             r.style.hideObjectOnPrint = hideRests
             # environLocal.printDebug(['makeRests(): add rests', r, r.duration])
             # place at oLowTarget to reach to oLow
-            v.insert(oLowTarget, r)
+            component.insert(oLowTarget, r)
 
         # create rest from end to highest
         qLen = oHighTarget - oHigh
-        # environLocal.printDebug(['v', v, oHigh, oHighTarget, 'qLen', qLen])
         if qLen > 0:
             r = note.Rest()
             r.duration.quarterLength = qLen
             r.style.hideObjectOnPrint = hideRests
             # place at oHigh to reach to oHighTarget
-            v.insert(oHigh, r)
+            component.insert(oHigh, r)
 
         if fillGaps:
-            gapStream = v.findGaps()
+            gapStream = component.findGaps()
             if gapStream is not None:
                 for e in gapStream:
                     r = note.Rest()
                     r.duration.quarterLength = e.duration.quarterLength
                     r.style.hideObjectOnPrint = hideRests
-                    v.insert(e.offset, r)
-        # environLocal.printDebug(['post makeRests show()', v])
+                    component.insert(e.offset, r)
 
     if returnObj.hasMeasures():
         # split rests at measure boundaries
@@ -905,14 +951,6 @@ def makeRests(
             returnObj.setElementOffset(m, accumulatedTime)
             accumulatedTime += m.highestTime
 
-            # process voices
-            for v in m.voices:
-                v.makeRests(inPlace=True,
-                            fillGaps=fillGaps,
-                            hideRests=hideRests,
-                            refStreamOrTimeRange=m,
-                            )
-
     if inPlace is not True:
         return returnObj
 


Test patch:
diff --git a/music21/stream/tests.py b/music21/stream/tests.py
index d6cf35b51..a2254a571 100644
--- a/music21/stream/tests.py
+++ b/music21/stream/tests.py
@@ -1989,6 +1989,44 @@ class Test(unittest.TestCase):
         p.makeRests(inPlace=True)
         self.assertEqual(p.duration.quarterLength, 8.0)
 
+    def testMakeRestsInMeasuresWithVoices(self):
+        p = Part()
+        m = Measure(meter.TimeSignature('4/4'), number=1)
+        v1 = Voice(note.Note(quarterLength=3.5))
+        v2 = Voice(note.Note(quarterLength=3.75))
+        m.insert(0, v1)
+        m.insert(0, v2)
+        p.insert(0, m)
+
+        post = p.makeRests(inPlace=False, timeRangeFromBarDuration=True)
+
+        # No loose rests outside voices
+        self.assertEqual(len(post.first().getElementsByClass(note.Rest)), 0)
+        # Total of two rests, one in each voice
+        self.assertEqual(len(post.recurse().getElementsByClass(note.Rest)), 2)
+
+        # Wrap into Score
+        sc = Score([p])
+        post = sc.makeRests(inPlace=False, timeRangeFromBarDuration=True)
+        # No loose rests outside parts
+        self.assertEqual(len(post.first().getElementsByClass(note.Rest)), 0)
+        # ... or outside measures
+        self.assertEqual(len(post.first().measure(1).getElementsByClass(note.Rest)), 0)
+        # Total of two rests, one in each voice
+        self.assertEqual(len(post.recurse().getElementsByClass(note.Rest)), 2)
+
+    def testMakeRestsByMakingVoices(self):
+        # Create incomplete measure with overlaps, like a MIDI file
+        m = Measure(meter.TimeSignature('4/4'), number=1)
+        m.insert(0, note.Note(quarterLength=3.5))
+        m.insert(0, note.Note(quarterLength=3.75))
+        m.makeVoices(inPlace=True)
+
+        # No loose rests outside voices
+        self.assertEqual(len(m.getElementsByClass(note.Rest)), 0)
+        # Total of two rests, one in each voice. Recursive search.
+        self.assertEqual(len(m[note.Rest]), 2)
+
     def testMakeMeasuresInPlace(self):
         sScr = Stream()
         sScr.insert(0, clef.TrebleClef())