You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Saving a stream as musicxml alters the stream #1557
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.
frommusic21importconverterpath="La_Campanella.mxl"song=converter.parse(path, forceSource=True).expandRepeats().stripTies()
a= ([m.offsetforminsong.flat.getElementsByClass('Note')]) # Note is just an example class, other's are affected too.song.write("musicxml", "/tmp/_.musicxml")
b= ([m.offsetforminsong.flat.getElementsByClass('Note')])
fora_, b_inzip(a, b):
ifa_!=b_:
print(a_, b_)
Expected vs. actual behavior
Expected: No output
Actual:
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
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
ifisinstance(obj, stream.Stream) andself.makeNotation:
obj=obj.makeRests(refStreamOrTimeRange=[0.0, obj.highestTime],
fillGaps=True,
inPlace=False,
hideRests=True, # just to fill up MusicXML displaytimeRangeFromBarDuration=True,
)
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:
Replacing m.highestTime with m.barDuration.quarterLength fixes this issue for me.
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!
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.
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.
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.
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.
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):
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.
Activity
mscuthbert commentedon May 4, 2023
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 commentedon May 7, 2023
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:
I have three findings:
music21/music21/musicxml/m21ToXml.py
Lines 498 to 504 in 696545a
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
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
Replacing
m.highestTimewithm.barDuration.quarterLengthfixes this issue for me.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 commentedon May 8, 2023
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:
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 commentedon May 8, 2023
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 commentedon May 8, 2023
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 commentedon May 9, 2023
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.
Avoid altering score on write cuthbertLab#1557
TimFelixBeyer commentedon May 9, 2023
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.highestTimeequal tom.barDuration.quarterLength.See this example (which is fixed if we replace
m.highestTimewithm.barDuration.quarterLength):Output
Avoid altering score on write
Avoid altering score on write
jacobtylerwalls commentedon May 11, 2023
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.