Repository: cuthbertLab/music21 (python)

Issue:
GuitarPro file cannot be parsed in 6.7.0 -- spanner contains same element twice
**music21 version**

`6.7.0`


**Problem summary**
The GuitarPro file provided in a list message about grace notes and lyrics can't be parsed after a regression in 01597856f0ad7e4f113c05aab27706625aed82f8 (feature using templates instead of deepcopies for joinable groups) when serializing fails. Workaround is to use `storePickle=False` so that a pickle isn't written.

**Steps to reproduce**

[frag.xml.zip](https://github.com/cuthbertLab/music21/files/6258453/frag.xml.zip)

```
>>> converter.parse('frag.xml')
converter: Loading original version
converter: Freezing Pickle
Traceback (most recent call last):
  File "/Users/jwalls/music21/music21/sites.py", line 923, in remove
    del self.siteDict[siteId]
KeyError: 4963930560

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/jwalls/music21/music21/converter/__init__.py", line 1167, in parse
    return parseFile(valueStr, number=number, format=m21Format,
  File "/Users/jwalls/music21/music21/converter/__init__.py", line 1027, in parseFile
    v.parseFile(fp, number=number, format=format, forceSource=forceSource, **keywords)
  File "/Users/jwalls/music21/music21/converter/__init__.py", line 572, in parseFile
    sf.write(fp=fpPickle, zipType='zlib')
  File "/Users/jwalls/music21/music21/freezeThaw.py", line 654, in write
    storage = self.packStream(self.stream)
  File "/Users/jwalls/music21/music21/freezeThaw.py", line 242, in packStream
    self.setupSerializationScaffold(streamObj)
  File "/Users/jwalls/music21/music21/freezeThaw.py", line 295, in setupSerializationScaffold
    subSF.setupSerializationScaffold()
  File "/Users/jwalls/music21/music21/freezeThaw.py", line 302, in setupSerializationScaffold
    self.setupStoredElementOffsetTuples(streamObj)
  File "/Users/jwalls/music21/music21/freezeThaw.py", line 466, in setupStoredElementOffsetTuples
    e.sites.remove(streamObj)
  File "/Users/jwalls/music21/music21/sites.py", line 927, in remove
    raise SitesException(
music21.sites.SitesException: an entry for this object (<music21.stream.SpannerStorage 0x127df91c0>) is not stored in this Sites object
```

**More information**
At a glance, it looks like after 01597856f0ad7e4f113c05aab27706625aed82f8 there are some missing spanners in the lower part. They're gathered on musicxml export but not when serializing or getting a text dump from `.show('text')`, so that may have been how we missed this.  Probably just need to gather missing spanners again before serializing.


Pull request:
Avoid leaving malformed StaffGroups in cases where ScoreExporter is used directly and add safety to `Stream.replace()`
**Before**
Streams with piano (joinable groups) could only be exported from `ScoreExporter` once. Fine for the typical usage of `Stream.write('musicxml')` since we make a deepcopy before reaching `ScoreExporter`. So the failure to export a second time only affected advanced users using `ScoreExporter` directly.

This limitation in the original feature was really a shortcut to get `<part-group type="stop" />` written. What I didn't realize until the TestExternal failure I saw in #938 is that this workflow was leaving side effects on the input stream -- which is not great -- and prevents exporting even after creating a new `ScoreExporter` object.

**Now**
Implement the feature in the right place! We would have had to revisit this eventually when adding a `makeNotation=False` for MusicXML export.

**Update**
Fixes #954  --  decided to raise `StreamException` in `Stream.replace()` if the replacement already exists, but otherwise, for `SpannerStorage` to be permissive, subclass replace() to just remove the target and quit. This is because SpannerStorage doesn't have offsets. Can't be that permissive in regular streams. Where exactly does the replacement go?

Turned out to be related work (same test demoing an exception in the partStaffExporter that we no longer raise had to be fiddled with), so I'm putting them on the same branch to ensure the tests pass with both fixes.

Patch:
diff --git a/music21/_version.py b/music21/_version.py
index 7af8793f9..420e32879 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, 3)  # can be 4-tuple: (7, 0, 5, 'a2')
+__version_info__ = (7, 0, 4)  # 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 3cb5a73ea..7e59f8f62 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.3'
+'7.0.4'
 
 Alternatively, after doing a complete import, these classes are available
 under the module "base":
diff --git a/music21/duration.py b/music21/duration.py
index 8c6318016..051d1f7a3 100644
--- a/music21/duration.py
+++ b/music21/duration.py
@@ -1821,8 +1821,7 @@ class Duration(prebase.ProtoM21Object, SlottedObjectMixin):
         True
         '''
         newTuplet.frozen = True
-        self.tuplets = self.tuplets + (newTuplet,)
-        self.informClient()
+        self.tuplets = self._tuplets + (newTuplet,)
 
     def augmentOrDiminish(self, amountToScale, retainComponents=False):
         '''
@@ -2845,7 +2844,8 @@ class Duration(prebase.ProtoM21Object, SlottedObjectMixin):
     @property
     def tuplets(self) -> Tuple[Tuplet, ...]:
         '''
-        return a tuple of Tuplet objects
+        Return a tuple of Tuplet objects.
+        Setting tuplets will inform the client (Note) that the duration has changed.
         '''
         if self._componentsNeedUpdating:
             self._updateComponents()
@@ -2856,6 +2856,7 @@ class Duration(prebase.ProtoM21Object, SlottedObjectMixin):
         # environLocal.printDebug(['assigning tuplets in Duration', tupletTuple])
         self._tuplets = tuple(tupletTuple)
         self._quarterLengthNeedsUpdating = True
+        self.informClient()
 
     def aggregateTupletMultiplier(self) -> OffsetQL:
         '''
@@ -3590,7 +3591,7 @@ class Test(unittest.TestCase):
         self.assertEqual(str(d.components),
                          "(DurationTuple(type='eighth', dots=0, quarterLength=0.5),)")
         self.assertFalse(d._componentsNeedUpdating)
-        self.assertTrue(d._quarterLengthNeedsUpdating)
+        self.assertFalse(d._quarterLengthNeedsUpdating)
         self.assertEqual(repr(d.quarterLength), 'Fraction(1, 3)')
         self.assertEqual(str(unitSpec(d)), "(Fraction(1, 3), 'eighth', 0, 3, 2, 'eighth')")
 
diff --git a/music21/harmony.py b/music21/harmony.py
index 20345c1bc..ae2f5ad6a 100644
--- a/music21/harmony.py
+++ b/music21/harmony.py
@@ -1387,8 +1387,8 @@ class ChordSymbol(Harmony):
     chords, by default appear as chord symbols in a score and have duration of
     0.
 
-    To obtain the chord representation of the in the score, change the
-    :attr:`music21.harmony.ChordSymbol.writeAsChord` to True. Unless otherwise
+    To obtain the chord representation of the `ChordSymbol` in the score, change
+    :attr:`~music21.harmony.Harmony.writeAsChord` to True. Unless otherwise
     specified, the duration of this chord object will become 1.0. If you have a
     leadsheet, run :meth:`music21.harmony.realizeChordSymbolDurations` on the
     stream to assign the correct (according to offsets) duration to each
diff --git a/music21/musicxml/m21ToXml.py b/music21/musicxml/m21ToXml.py
index 04ee237aa..7cea802de 100644
--- a/music21/musicxml/m21ToXml.py
+++ b/music21/musicxml/m21ToXml.py
@@ -1353,6 +1353,8 @@ class ScoreExporter(XMLExporterBase, PartStaffExporterMixin):
 
         self.partExporterList: List['PartExporter'] = []
 
+        self.joinedGroups: List['StaffGroup'] = []
+
         self.instrumentList = []
         self.midiChannelList = []
 
@@ -1929,6 +1931,20 @@ class ScoreExporter(XMLExporterBase, PartStaffExporterMixin):
           <score-part id="P4">...
           <part-group number="1" type="stop" />
         </part-list>
+
+        And with a piano staff, to observe that only one <part> tag has both "start" and "stop":
+
+        >>> cpe = corpus.parse('cpebach')
+        >>> SX = musicxml.m21ToXml.ScoreExporter(cpe)
+        >>> SX.scorePreliminaries()
+        >>> SX.parsePartlikeScore()
+
+        >>> mxPartList = SX.setPartList()
+        >>> SX.dump(mxPartList)
+        <part-list>
+          <part-group number="1" type="start">...
+          <part-group number="1" type="stop" />
+        </part-list>
         '''
 
         spannerBundle = self.spannerBundle
@@ -1959,7 +1975,10 @@ class ScoreExporter(XMLExporterBase, PartStaffExporterMixin):
             # check for last
             activeIndex = None
             for sg in staffGroups:
-                if sg.isLast(p):
+                # Handle last part in the StaffGroup
+                # as well as the first part in the StaffGroup if it was joined to the others
+                # In that latter case, we need the <part-group type="stop" /> on the FIRST m21 part
+                if (sg.isLast(p) or (sg in self.joinedGroups and sg.isFirst(p))):
                     # find the spanner in the dictionary already-assigned
                     for key, value in partGroupIndexRef.items():
                         if value is sg:
diff --git a/music21/musicxml/partStaffExporter.py b/music21/musicxml/partStaffExporter.py
index 45b85a1f5..33e0bc05b 100644
--- a/music21/musicxml/partStaffExporter.py
+++ b/music21/musicxml/partStaffExporter.py
@@ -131,7 +131,8 @@ class PartStaffExporterMixin:
         >>> len(staffTags)
         2
         '''
-        for group in self.joinableGroups():
+        self.joinedGroups = self.joinableGroups()
+        for group in self.joinedGroups:
             self.addStaffTagsMultiStaffParts(group)
             self.movePartStaffMeasureContents(group)
             self.setEarliestAttributesAndClefsPartStaff(group)
@@ -283,13 +284,6 @@ class PartStaffExporterMixin:
             <staff>2</staff>
           </note>
         </measure>
-
-        Fails if attempted a second time:
-
-        >>> root = SX.parse()
-        Traceback (most recent call last):
-        music21.musicxml.xmlObjects.MusicXMLExportException:
-            In part (MusicXML Part), measure (1): Attempted to create a second <staff> tag
         '''
         initialPartStaffRoot: Optional[Element] = None
         for i, ps in enumerate(group):
@@ -595,10 +589,6 @@ class PartStaffExporterMixin:
         by a single :class:`~music21.musicxml.m21ToXml.PartExporter`, remove any
         obsolete `PartExporter` from `self.partExporterList`.
 
-        In addition, remove any obsolete `PartStaff` from the `StaffGroup`
-        (in the deepcopied stream used for exporting) to ensure <part-group type="stop" />
-        is written.
-
         Called by :meth:`joinPartStaffs`
 
         >>> from music21.musicxml import testPrimitive
@@ -611,9 +601,6 @@ class PartStaffExporterMixin:
         >>> SX.postPartProcess()
         >>> len(SX.partExporterList)
         1
-        >>> partGroupStop = SX.xmlRoot.findall('.//part-group')[1]
-        >>> SX.dump(partGroupStop)
-        <part-group number="1" type="stop" />
         '''
         for ps in group[1:]:
             partStaffRoot: Element = self.getRootForPartStaff(ps)
@@ -621,8 +608,6 @@ class PartStaffExporterMixin:
             # noinspection PyAttributeOutsideInit
             self.partExporterList = [pex for pex in self.partExporterList
                                         if pex.xmlRoot != partStaffRoot]
-            # Replace PartStaff in StaffGroup -- ensures <part-group number="1" type="stop" />
-            group.replaceSpannedElement(ps, group.getFirst())
 
     @staticmethod
     def moveMeasureContents(measure: Element, otherMeasure: Element, staffNumber: int):
@@ -945,6 +930,18 @@ class Test(unittest.TestCase):
         SX.parsePartlikeScore()
         self.assertEqual(len(SX.joinableGroups()), 1)
 
+    def testJoinPartStaffsAgain(self):
+        '''
+        Regression test for side effects on the stream passed to ScoreExporter
+        preventing it from being written out again.
+        '''
+        from music21 import corpus
+        from music21.musicxml.m21ToXml import ScoreExporter
+        b = corpus.parse('cpebach')
+        SX = ScoreExporter(b)
+        SX.parse()
+        SX.parse()
+
 
 if __name__ == '__main__':
     import music21
diff --git a/music21/musicxml/xmlToM21.py b/music21/musicxml/xmlToM21.py
index 781f39705..7601297c7 100644
--- a/music21/musicxml/xmlToM21.py
+++ b/music21/musicxml/xmlToM21.py
@@ -1984,6 +1984,17 @@ class PartParser(XMLParserBase):
 
         sets self.lastMeasureWasShort to True or False if it is an incomplete measure
         that is not a pickup.
+
+        >>> m = stream.Measure([meter.TimeSignature('4/4'), harmony.ChordSymbol('C7')])
+        >>> m.highestTime
+        0.0
+        >>> PP = musicxml.xmlToM21.PartParser()
+        >>> PP.setLastMeasureInfo(m)
+        >>> PP.adjustTimeAttributesFromMeasure(m)
+        >>> m.highestTime
+        4.0
+        >>> PP.lastMeasureWasShort
+        False
         '''
         # note: we cannot assume that the time signature properly
         # describes the offsets w/n this bar. need to look at
@@ -1999,7 +2010,9 @@ class PartParser(XMLParserBase):
         if mHighestTime >= lastTimeSignatureQuarterLength:
             mOffsetShift = mHighestTime
 
-        elif mHighestTime == 0.0 and not m.recurse().notesAndRests:
+        elif (mHighestTime == 0.0
+              and not m.recurse().notesAndRests.getElementsNotOfClass('Harmony')
+              ):
             # this routine fixes a bug in PDFtoMusic and other MusicXML writers
             # that omit empty rests in a Measure.  It is a very quick test if
             # the measure has any notes.  Slower if it does not.
@@ -2007,6 +2020,7 @@ class PartParser(XMLParserBase):
             r.duration.quarterLength = lastTimeSignatureQuarterLength
             m.insert(0.0, r)
             mOffsetShift = lastTimeSignatureQuarterLength
+            self.lastMeasureWasShort = False
 
         else:  # use time signature
             # for the first measure, this may be a pickup
@@ -3808,7 +3822,7 @@ class MeasureParser(XMLParserBase):
         # add a reference of this note to this spanner
         if target is not None:
             su.addSpannedElements(target)
-        # environLocal.printDebug(['adding n', n, id(n), 'su.getSpannedElements',
+        # environLocal.printDebug(['adding n', target, id(target), 'su.getSpannedElements',
         #     su.getSpannedElements(), su.getSpannedElementIds()])
         if mxObj.get('type') == 'stop':
             su.completeStatus = True
diff --git a/music21/stream/base.py b/music21/stream/base.py
index 4d8f92316..a826ca9b9 100644
--- a/music21/stream/base.py
+++ b/music21/stream/base.py
@@ -2599,7 +2599,8 @@ class Stream(core.StreamCoreMixin, base.Music21Object):
         Given a `target` object, replace it with
         the supplied `replacement` object.
 
-        Does nothing if target cannot be found.
+        Does nothing if target cannot be found. Raises StreamException if replacement
+        is already in the stream.
 
         If `allDerived` is True (as it is by default), all sites (stream) that
         this this stream derives from and also
@@ -2668,6 +2669,8 @@ class Stream(core.StreamCoreMixin, base.Music21Object):
         in stream twice.  recurse and shiftOffsets changed to keywordOnly arguments
 
         Changed in v6 -- recurse works
+
+        Changed in v7 -- raises StreamException if replacement is already in the stream.
         '''
         def replaceDerived(startSite=self):
             if not allDerived:
@@ -2680,6 +2683,14 @@ class Stream(core.StreamCoreMixin, base.Music21Object):
                                         recurse=recurse,
                                         allDerived=False)
 
+        try:
+            i = self.index(replacement)
+        except StreamException:
+            # good. now continue.
+            pass
+        else:
+            raise StreamException(f'{replacement} already in {self}')
+
         try:
             i = self.index(target)
         except StreamException:
@@ -7882,8 +7893,7 @@ class Stream(core.StreamCoreMixin, base.Music21Object):
             return self._cache['Duration']
         else:
             # environLocal.printDebug(['creating new duration based on highest time'])
-            self._cache['Duration'] = duration.Duration()
-            self._cache['Duration'].quarterLength = self.highestTime
+            self._cache['Duration'] = duration.Duration(quarterLength=self.highestTime)
             return self._cache['Duration']
 
     def _setDuration(self, durationObj):
@@ -13911,6 +13921,25 @@ class SpannerStorage(Stream):
     def coreStoreAtEnd(self, element, setActiveSite=True):  # pragma: no cover
         raise StreamException('SpannerStorage cannot store at end.')
 
+    def replace(self,
+                target: base.Music21Object,
+                replacement: base.Music21Object,
+                *,
+                recurse: bool = False,
+                allDerived: bool = True) -> None:
+        '''
+        Overrides :meth:`~music21.stream.Stream.replace` in order to check first
+        whether `replacement` already exists in `self`. If so, delete `target` from
+        `self` and return; otherwise call the superclass method.
+
+        New in v7.
+        '''
+        # Does not perform a recursive search, but shouldn't need to
+        if replacement in self:
+            self.remove(target)
+            return
+        super().replace(target, replacement, recurse=recurse, allDerived=allDerived)
+
 
 class VariantStorage(Stream):
     '''


Test patch:
diff --git a/music21/stream/tests.py b/music21/stream/tests.py
index 679cc8007..1ab478bd5 100644
--- a/music21/stream/tests.py
+++ b/music21/stream/tests.py
@@ -545,6 +545,24 @@ class Test(unittest.TestCase):
         self.assertEqual(a.duration.quarterLength, 2.0)
         self.assertEqual(a.highestTime, 4)
 
+    def testStreamDurationRecalculated(self):
+        from fractions import Fraction
+
+        a = Stream()
+        n = note.Note(quarterLength=1.0)
+        a.append(n)
+        self.assertEqual(a.duration.quarterLength, 1.0)
+
+        tup = duration.Tuplet()
+        n.duration.tuplets = (tup,)
+        self.assertEqual(a.duration.quarterLength, Fraction(2, 3))
+
+        # Also (regression) test clearing the cache
+        # https://github.com/cuthbertLab/music21/issues/957
+        n.duration.tuplets = (tup, tup)
+        a.coreElementsChanged()
+        self.assertEqual(a.duration.quarterLength, Fraction(4, 9))
+
     def testMeasureStream(self):
         '''An approach to setting TimeSignature measures in offsets and durations
         '''
@@ -1181,6 +1199,9 @@ class Test(unittest.TestCase):
         stripped = s.stripTies(inPlace=False)
         sn1 = stripped.flat.notes[0]
 
+        self.assertEqual(len(stripped.spanners[0]), 1)
+        self.assertEqual(len(stripped.spanners[1]), 1)
+
         self.assertTrue(stripped.spanners[0].isFirst(sn1))
         self.assertTrue(stripped.spanners[0].isLast(sn1))
         self.assertTrue(stripped.spanners[1].isFirst(sn1))
@@ -2105,6 +2126,10 @@ class Test(unittest.TestCase):
         s.replace(n4, n1)
         self.assertEqual([s[0], s[1]], [n3, n1])
 
+        error_msg = f'{n3} already in {s}'
+        with self.assertRaises(StreamException, msg=error_msg):
+            s.replace(n4, n3)
+
     def testReplaceA1(self):
         sBach = corpus.parse('bach/bwv324.xml')
         partSoprano = sBach.parts.first()