Saving a stream as musicxml alters the stream

[TimFelixBeyer]

music21 version

8.1.0

Problem summary
When saving a Stream s to musicxml, the object s is altered after saving to disk.
The documentation does not mention this side effect, in fact quite the opposite: "Some formats, including .musicxml, create a copy of the stream, pack it into a well-formed score if necessary, and run makeNotation()." which suggests that the original stream is left unaltered.

Steps to reproduce
Get e.g., this .mxl file: https://musescore.com/user/27439014/scores/5316979

from music21 import converter
path = "La_Campanella.mxl"
song = converter.parse(path, forceSource=True).expandRepeats().stripTies()
a = ([m.offset for m in song.flat.getElementsByClass('Note')])  # Note is just an example class, other's are affected too.
song.write("musicxml", "/tmp/_.musicxml")
b = ([m.offset for m in song.flat.getElementsByClass('Note')])
for a_, b_ in zip(a, b):
    if a_ != b_:
        print(a_, b_)

Expected vs. actual behavior
Expected: No output
Actual:

87.5 88.0
87.75 88.25
88.0 88.5
88.25 88.75
88.5 89.0
88.75 89.25
89.0 89.5
89.5 90.0
90.0 90.5
90.5 91.0
92.5 93.0
93.0 93.5
93.5 94.0
93.75 94.25
...

More information
I traced the changes and believe these lines are responsible:

music21/music21/stream/makeNotation.py

Lines 852 to 858 in 696545a

	inner_part.makeRests(
	inPlace=True,
	fillGaps=fillGaps,
	hideRests=hideRests,
	refStreamOrTimeRange=refStreamOrTimeRange,
	timeRangeFromBarDuration=timeRangeFromBarDuration,
	)

Please let me know if the behaviour is intended.
Thanks for your help!

[mscuthbert]

Not intended! Can you reduce the score to a minimum test set that displays the behavior? Does it do this if you leave out the stripTies? What happens around offset 87 that doesn't happen before in the piece?

I ran into another case where showing something changed the original the other day, but unrelated to this. Show should not alter any non-private/"core" attributes

[TimFelixBeyer]

I believe the problem occurs at notes that are tied across two measures (that's where La_Campanella.mxl goes awry as well).
Here's a small example:

from music21 import stream, note, meter, tie

s = stream.Score(id='mainScore')
p0 = stream.Part(id='part0')

m01 = stream.Measure(number=1)
m01.append(meter.TimeSignature('4/4'))
d1 = note.Note('D', type="half", dots=1)
c1 = note.Note('C', type="quarter")
c1.tie = tie.Tie("start")
m01.append([d1, c1])
m02 = stream.Measure(number=2)
c2 = note.Note('C', type="quarter")
c2.tie = tie.Tie("stop")
c3 = note.Note("D", type="half")
m02.append([c2, c3])
p0.append([m01, m02])

s.insert(0, p0)
s = s.stripTies()

prev = sorted([e for e in s.flat], key=lambda x: x.offset)

for i, p in enumerate(prev):
    print(f"{p.offset}, {p.measureNumber}, {p}, {p.duration.quarterLength}")

s.write("musicxml", "/tmp/_.mxl")
after = sorted([e for e in s.flat], key=lambda x: x.offset)

for i, p in enumerate(after):
    print(f"{p.offset}, {p.measureNumber}, {p}, {p.duration.quarterLength}")

I have three findings:

1. New rests are added to the original stream, not just the written copy (this is not surprising, given that the rests are added in place):

   music21/music21/musicxml/m21ToXml.py

   Lines 498 to 504 in 696545a

   	if isinstance(obj, stream.Stream) and self.makeNotation:
   	obj.makeRests(refStreamOrTimeRange=[0.0, obj.highestTime],
   	fillGaps=True,
   	inPlace=True,
   	hideRests=True, # just to fill up MusicXML display
   	timeRangeFromBarDuration=True,
   	)

   
   This occurs because since v8, rests are added before makeNotation, which makes a copy of the stream.
   A simple fix for that would be changing the lines to

if isinstance(obj, stream.Stream) and self.makeNotation:
    obj = obj.makeRests(refStreamOrTimeRange=[0.0, obj.highestTime],
                        fillGaps=True,
                        inPlace=False,
                        hideRests=True,  # just to fill up MusicXML display
                        timeRangeFromBarDuration=True,
                        )

2. makeRests alters the offsets of the elements already in the score. If I understand the purpose of the method correctly, it should never do that.
   I believe the reason for this shift is found here:
   

   music21/music21/stream/makeNotation.py

   Lines 980 to 983 in 696545a

   	accumulatedTime = 0.0
   	for m in returnObj.getElementsByClass(stream.Measure):
   	returnObj.setElementOffset(m, accumulatedTime)
   	accumulatedTime += m.highestTime

   
   Replacing m.highestTime with m.barDuration.quarterLength fixes this issue for me.

3. I think makeRests inserts unnecessary rests. The example score above does not require the first rest to be rendered correctly, yet it is added nonetheless:

Feel free to let me know what you think of the proposed changes!

[jacobtylerwalls]

Thanks for looking into this!

1
Agree with the problem diagnosis. I missed this side-effect in #1361. I think you'll also want to avoid subsequent unnecessary deep copies, though, so something like:

diff --git a/music21/musicxml/m21ToXml.py b/music21/musicxml/m21ToXml.py
index c71de9596..859d1354c 100644
--- a/music21/musicxml/m21ToXml.py
+++ b/music21/musicxml/m21ToXml.py
@@ -496,9 +496,10 @@ class GeneralObjectExporter:
         outObj = None
 
         if isinstance(obj, stream.Stream) and self.makeNotation:
-            obj.makeRests(refStreamOrTimeRange=[0.0, obj.highestTime],
+            # This is where the first (and hopefully only) copy is made.
+            obj = obj.makeRests(refStreamOrTimeRange=[0.0, obj.highestTime],
                           fillGaps=True,
-                          inPlace=True,
+                          inPlace=False,
                           hideRests=True,  # just to fill up MusicXML display
                           timeRangeFromBarDuration=True,
                           )
@@ -517,9 +518,9 @@ class GeneralObjectExporter:
 
     def fromScore(self, sc):
         '''
-        Copies the input score and runs :meth:`~music21.stream.Score.makeNotation` on the copy.
+        Runs :meth:`~music21.stream.Score.makeNotation` on the copy.
         '''
-        scOut = sc.makeNotation(inPlace=False)
+        scOut = sc.makeNotation(inPlace=True)
         if not scOut.isWellFormedNotation():
             warnings.warn(f'{scOut} is not well-formed; see isWellFormedNotation()',
                 category=MusicXMLWarning)
@@ -548,7 +549,7 @@ class GeneralObjectExporter:
         solutions in Part or Stream production.
         '''
         m.coreGatherMissingSpanners()
-        mCopy = m.makeNotation()
+        mCopy = m.makeNotation(inPlace=True)

2
What about streams without time signatures? Do you mind presenting the case that doesn't behave as you expect in more detail? Thanks.

3
Your example is a score that has had stripTies() run on it, yielding an overfilled first measure and an underfilled second measure. I think I would expect that making rests on that would fill the second measure.

If you want to display the stripped ties score (are you sure you want to display that?) then you could makeTies() on it first, which restores the output I think you're expecting.

---

Do you have an interest in preparing a PR for 1?

[jacobtylerwalls]

This would be a new feature, but maybe the musicxml show pipeline could check to see if the most recent derivation was stripTies, and then makeTies() first.

[mscuthbert]

This would be a new feature, but maybe the musicxml show pipeline could check to see if the most recent derivation was stripTies, and then makeTies() first.

I don't think I want to go down this rabbit hole. stripTies() is an analysis function, there are tons of analysis functions that output cannot recover from so I don't want to set the precedent that we're supposed to be able to do that.

[TimFelixBeyer]

I don't think I want to go down this rabbit hole. stripTies() is an analysis function, there are tons of analysis functions that output cannot recover from so I don't want to set the precedent that we're supposed to be able to do that.

I understand - it might be good to let the users know that they are responsible for not over/underfilling bars.
Music21 does take care of some "misformatting" to render scores (like adding rests), so it's not obvious where that begins and ends.

[TimFelixBeyer]

2 What about streams without time signatures? Do you mind presenting the case that doesn't behave as you expect in more detail? Thanks.

As far as I can tell, (at least if programmatically created), without time signatures, measures expand to accept whatever notes are in them, making m.highestTime equal to m.barDuration.quarterLength.

See this example (which is fixed if we replace m.highestTime with m.barDuration.quarterLength):

from music21 import stream, note, meter, tie

s = stream.Score(id='mainScore')
p0 = stream.Part(id='part0')

m01 = stream.Measure(number=1)
m01.append(meter.TimeSignature('4/4'))
d1 = note.Note('D', type="half", dots=1)
c1 = note.Note('C', type="quarter")
c1.tie = tie.Tie("start")
m01.append([d1, c1])
m02 = stream.Measure(number=2)
c2 = note.Note('C', type="quarter")
c2.tie = tie.Tie("stop")
c3 = note.Note("D", type="half")
m02.append([c2, c3])
p0.append([m01, m02])

s.insert(0, p0)
s = s.stripTies()

for i, p in enumerate(s.flatten()):
    print(f"{p.offset}, {p.measureNumber}, {p}, {p.duration.quarterLength}")
s = s.makeRests(refStreamOrTimeRange=[0, 8.0], fillGaps=True, inPlace=False, timeRangeFromBarDuration=True)
print("---------------")
for i, p in enumerate(s.flatten()):
    print(f"{p.offset}, {p.measureNumber}, {p}, {p.duration.quarterLength}")

Output

0.0, 1, <music21.meter.TimeSignature 4/4>, 0.0
0.0, 1, <music21.note.Note D>, 3.0
3.0, 1, <music21.note.Note C>, 2.0
5.0, 2, <music21.note.Note D>, 2.0
---------------
0.0, 1, <music21.meter.TimeSignature 4/4>, 0.0
0.0, 1, <music21.note.Note D>, 3.0
3.0, 1, <music21.note.Note C>, 2.0
5.0, 2, <music21.note.Rest quarter>, 1.0 <--- this offset should be 4.0
6.0, 2, <music21.note.Note D>, 2.0  <--- this offset should not change to 6.0
8.0, 2, <music21.note.Rest quarter>, 1.0  <--- this offset should be 7.0, not 8.0

[jacobtylerwalls]

See this example (which is fixed if we replace m.highestTime with m.barDuration.quarterLength):

Feel free to move this discussion point to a new issue. The reason I added measure repositioning to makeRests at all was in #922 to handle the case where underfilled measures are expanded to their bar length; without repositioning, you'd be left with measure overlap. You've presented here the converse, where overfilled measures, once repositioned to not overlap, produce an unintended musical result.

I think your proposal makes sense. A regression test would be great.