Closed mdsjazz-kena closed 3 years ago
mscuthbert
commented
3 years ago We're going to need a much more minimal example if the fix is to become a priority -- I can't debug through an entire Rachmaninoff Prelude to see what might be causing the problem. Can you remove all but that one measure and reexport as uncompressed xml?
jacobtylerwalls
commented
3 years ago I'm here and caffeinated, so I just unzipped it myself.
Measure 6, staff 1, voice 2, plus the backup tag, plus the first rest in voice 5
<note default-x="15.60" default-y="-80.00" dynamics="95.56">
<pitch>
<step>D</step>
<octave>3</octave>
</pitch>
<duration>18</duration>
<voice>2</voice>
<type>quarter</type>
<dot/>
<stem>down</stem>
<staff>1</staff>
<notations>
<articulations>
<accent/>
</articulations>
</notations>
</note>
<note>
<rest/>
<duration>30</duration> <!--------- this is what m21 reads as 3.0 QL, but this is really 2.5QL
<voice>2</voice>
<type>half</type>
<dot/>
<staff>1</staff>
</note>
<backup>
<duration>48</duration> <!---------- this is correct, we've gone 48 (4.0QL) and we need to backup 4.0QL
</backup>
<note>
<rest/>
<duration>6</duration>
<voice>5</voice>
<type>eighth</type>
<staff>2</staff>
</note>I expect this is just an issue with trusting the note type (dotted half) instead of the actual <duration>, which xmlToDuration handles with an internal variable forceRaw. Try removing the <type> tags on that rest in the source and let us know how it turns out.
jacobtylerwalls
commented
3 years ago Making the use of raw durations configurable by the user with a keyword is intriguing.
Also -- as of 6.5 you should be able to just check the layout.StaffGroup objects (with symbol='brace') to find connected piano staves instead of resorting to a regex, in case helpful.
jacobtylerwalls
commented
3 years ago Hi @mdsjazz-kena , forgive me if my response was cryptic. What I recall finding when I looked into this is that your file had a discrepancy between the <duration> of 2.5QL which is the legal duration, versus the dotted half printed in the notation.
m21 prioritizes the printed notation (dotted half) over the legal <duration>. So a workaround would be to override xmlToDuration and always forceRaw or make it configurable with a keyword. Does that make sense?
A little fancy, but a TODO for m21 could be to:
xmlMeasureToMeasure, if we get overfilled or incomplete measures, try a second time with forceRaw=Truemscuthbert
commented
3 years ago I think that what we really SHOULD do is create unlinked durations for notes whose MusicXML duration in divisions does not correspond to the duration implied by the note. But this will slow down parsing for 99.9% of notes with benefit for only a few. I wish that there were a standardized tag within the "supports" tag that could say whether we might encounter these.
jacobtylerwalls
commented
3 years ago There's a chance this might actually run faster. I'm showing about a 1-5% improvement (depending on the tools used, I'm not sitting here for an hour to run 1m loops) if we just use the raw duration, and then get a DurationTuple from the cooked version and short circuit immediately if equal.
The overhead is probably saved from not having to clear and then readd the duration tuple in the cooked workflow. creating Duration objects from quarter length instead of a DurationTuple, it seems.
diff --git a/music21/musicxml/xmlToM21.py b/music21/musicxml/xmlToM21.py
index 781f39705..4fe603615 100644
--- a/music21/musicxml/xmlToM21.py
+++ b/music21/musicxml/xmlToM21.py
@@ -3234,8 +3234,9 @@ class MeasureParser(XMLParserBase):
forceRaw = True
# TODO: empty-placement
+ rawQL = qLen
# two ways to create durations, raw (from qLen) and cooked (from type, time-mod, dots)
- if forceRaw:
+ if True:
if d is not None:
# environLocal.printDebug(['forced to use raw duration', durRaw])
durRaw = duration.Duration() # raw just uses qLen
@@ -3258,11 +3259,15 @@ class MeasureParser(XMLParserBase):
# 'mxNote:',
# ET.tostring(mxNote, encoding='unicode'),
# 'last mxDivisions:', divisions])
- durRaw.quarterLength = qLenRounded
+ rawQL = durRaw.quarterLength = qLenRounded
else:
d = duration.Duration(quarterLength=qLen)
- else: # a cooked version builds up from pieces
+ # can't do this unless we have a type, so if not forceRaw
+ if not forceRaw: # a cooked version builds up from pieces
dt = duration.durationTupleFromTypeDots(durationType, numDots)
+ if dt.quarterLength == rawQL:
+ # equality passed, so we're done
+ return d if inputM21 is None else None
if d is not None:
d.clear()
d.addDurationTuple(dt)
timing script (can't use corpus.parse, because that doesn't accept writePickle=False):
from music21 import *
cmd = "converter.parse('music21/corpus/beach/prayer_of_a_tired_child.musicxml', forceSource=True, storePickle=False)"
import cProfile
cProfile.run(cmd)Something breaks with tuplets if I do this, so, of course, all bets are off timing-wise until we have a real patch. Not going to jump on this, but found it curious.
======================================================================
FAIL: testNestedTuplets (__main__.Test)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/jwalls/music21/music21/musicxml/xmlToM21.py", line 6571, in testNestedTuplets
self.assertEqual(repr(nList[i].duration.tuplets),
AssertionError: '(<music21.duration.Tuplet 15/8/256th>,)' != '(<music21.duration.Tuplet 3/2/eighth>, <music21.duration.Tuplet 5/2/eighth>)'
- (<music21.duration.Tuplet 15/8/256th>,)
+ (<music21.duration.Tuplet 3/2/eighth>, <music21.duration.Tuplet 5/2/eighth>)Also, is there a Claude Levi-Strauss pun underneath all this...? 🤣
jacobtylerwalls
commented
3 years ago Thanks for doing some investigation @mdsjazz-kena , I think this looks better -- do you agree?
PR #941
>>> s.measure(6).parts.first().show('t', addEndTimes=True)
{0.0 - 0.0} <music21.instrument.Instrument 'P1: Piano: Piano'>
{0.0 - 0.0} <music21.clef.TrebleClef>
{0.0 - 0.0} <music21.key.KeySignature of 2 flats>
{0.0 - 0.0} <music21.meter.TimeSignature 4/4>
{0.0 - 4.0} <music21.stream.Measure 6 offset=0.0>
{0.0 - 4.0} <music21.stream.Voice 1>
{0.0 - 0.5} <music21.note.Rest rest>
{0.5 - 0.75} <music21.chord.Chord D4 F#4 D5>
{0.75 - 1.0} <music21.chord.Chord D4 F#4 D5>
{1.0 - 1.5} <music21.chord.Chord D4 F#4 D5>
{1.5 - 1.75} <music21.note.Note F#>
{1.75 - 2.0} <music21.note.Note A>
{2.0 - 2.5} <music21.note.Note B->
{2.5 - 3.0} <music21.chord.Chord G3 D4 E4>
{3.0 - 3.5} <music21.note.Note G>
{3.5 - 4.0} <music21.chord.Chord B-3 D4 E4>
{0.0 - 4.0} <music21.stream.Voice 2>
{0.0 - 1.5} <music21.note.Note D>
{1.5 - 4.0} <music21.note.Rest rest>
{1.5 - 1.5} <music21.clef.BassClef>
{2.5 - 2.5} <music21.clef.TrebleClef>
{0.0 - 0.0} <music21.spanner.Slur <music21.note.Note F#><music21.note.Note B->>
jacobtylerwalls
commented
3 years ago I hope I didn't step on your interest in working on this area of the code! If you're interested in durations and voices and gaps, which is how I got started working on music21, let me know. You could help me think through #706 .
music21 version
6.1 through 6.5, most likely other versionsProblem summary
TLDR: When a MusicXML measure contains elements with more duration than should be specified by the time signature, rests are incorrectly created and appended to the beginning of the measures, with duration equal to the aforementioned duration discrepancy. Because of this, Music21 content no longer becomes equal to MusicXML content. More issues arise when the durations are shorter, but they are less important issues.
Steps to reproduce
I'm adding external files because the issue stems from bad XML files themselves, not anything that can easily be reproduced by creating a stream manually. Rachmaninoff_Prelude_Op._23_No._5_G_Minor.mxl.zip
Expected vs. actual behavior Expected: When Music21 reads in a bad MusicXML where elements have duration longer than the duration specified by the XML time signature, Music21 converts these measures and deals with the MusicXML errors however it chooses to, but does not add any incorrect or superfluous content to the stream measures when this occurs.
Actual: When Music21 reads in a bad MusicXML where elements have duration longer than the duration specified by the XML time signature, Music21 converts these measures and creates the elements as written with overextended duration, but also creates extra incorrect rest elements that were not in the XML, appended to the beginning of measures and voices.
More information
Long read, and my thought process: This shouldn't have to be a problem, but it's one I've unfortunately run into.
In a MusicXML, when the duration of a part's measure's elements extended beyond the expected duration of the measure, a funny phenomenon occurs in the XML to M21 conversion. If an instrument has multiple staves or voices, for every staff/voice that is "next" after an overextended measure, a rest is added to the beginning of the next staff/voice *equal to the duration by which the last staff/voice had been overextended. Here's a concrete example:
In an included piece by Rachmaninoff, measure 6, there are two staves and multiple voices per part. In the MusicXML:
In the M21 stream:
I didn't finish diagnosing the problem, but it occurs in music21/musicxml/xmltom21.py, around line 2320-2340 in measureParser.parse(), probably at the v.makeRests() function. Before this, the stream contains elements only from the MusicXML (as expected), but all elements (at least GeneralNotes) were offset by the amount of beats the last staff/voice went over by, e.g. a voice object looking like this.
My guess is that the coreInsert() uses some kind of info written in the XML to decide how far to backtrack the offset when creating streams, and in this case, it didn't back up far enough because the last measure's elements went over the expected duration. Then 4.5 current offset of the last voice's
After the v.makeRests() call, the voice would look like this:
and suddenly the stream has rest elements that don't match the content in the MusicXML.
I was able to write a work-around routine to deal with this. I use a regular expression to strip the staff label from the part ID's, so part ID's P1-Staff1 and P1-Staff2 both become P1, P1 and match each other. After this, I go sequentially through the measures/voices until I find at least one duration overshoot. After this, I delete the first element of every staff/voice remaining to that part, because I assume the element will be a superfluous rest.
Although this routine was easy enough to write, I write this report so that other people are aware of the issue and so maybe it can be rectified in the source code. I'd also love to explore this more and see if I can fix in source, if you happen to be busy. I've already spent a couple days with this problem.