Skip to content

Saving a stream as musicxml alters the stream #1557

Closed

Description

@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:

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!

Activity

  1. mscuthbert commented on May 4, 2023

    @mscuthbert
    Member

    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

  2. TimFelixBeyer commented on May 7, 2023

    @TimFelixBeyer
    ContributorAuthor

    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}")

    Screenshot 2023-05-07 at 14 43 15

    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):
      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,
                            )
    1. 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:

      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.

    2. 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:

    Screenshot 2023-05-07 at 20 19 50

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

  3. jacobtylerwalls commented on May 8, 2023

    @jacobtylerwalls
    Member

    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?

  4. jacobtylerwalls commented on May 8, 2023

    @jacobtylerwalls
    Member

    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.

  5. mscuthbert commented on May 8, 2023

    @mscuthbert
    Member

    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.

  6. TimFelixBeyer commented on May 9, 2023

    @TimFelixBeyer
    ContributorAuthor

    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.

  7. TimFelixBeyer commented on May 9, 2023

    @TimFelixBeyer
    ContributorAuthor

    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
    
  8. linked a pull request that will close this issueAvoid altering score on write #1560on May 11, 2023
  9. jacobtylerwalls commented on May 11, 2023

    @jacobtylerwalls
    Member

    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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions