Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interpret empty <forward> tags as rests on Finale documents only #1636

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Interpret empty <forward> tags as rests on Finale documents only #1636

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
de901e6
Fix music21 <forward> tag parsing
TimFelixBeyer Aug 20, 2023
b217557
Remove incorrectly added rests
TimFelixBeyer Aug 20, 2023
88871e6
lint
TimFelixBeyer Aug 20, 2023
273e9b1
Correctly deal with hidden rests and XML <forward>
TimFelixBeyer Aug 21, 2023
25afc26
Only run makeRests on MusicXML from Finale
TimFelixBeyer Dec 26, 2024
ab546f9
Merge branch 'master' into TimFelixBeyer-patch-1
TimFelixBeyer Dec 26, 2024
5bed7fc
Fix flake8
TimFelixBeyer Dec 26, 2024
26a83ac
Update hiddenRests test to be Finale only
TimFelixBeyer Dec 26, 2024
652b393
Add tests for finale and non-finale hiddenRest insertion
TimFelixBeyer Dec 26, 2024
cdf3359
Add finale-specific test for handling <forward> note tags
TimFelixBeyer Dec 26, 2024
ee708a2
fix whitespace
TimFelixBeyer Dec 26, 2024
2ecae3f
fix mypy
TimFelixBeyer Dec 26, 2024
7a1cc89
fix line length
TimFelixBeyer Dec 26, 2024
0a5a259
fix mypy
TimFelixBeyer Dec 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion music21/_version.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
'''
from __future__ import annotations

__version__ = '9.2.0b1'
__version__ = '9.2.0b2'

def get_version_tuple(vv):
v = vv.split('.')
Expand Down
2 changes: 1 addition & 1 deletion music21/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
<class 'music21.base.Music21Object'>

>>> music21.VERSION_STR
'9.2.0b1'
'9.2.0b2'

Alternatively, after doing a complete import, these classes are available
under the module "base":
Expand Down
2 changes: 1 addition & 1 deletion music21/musicxml/test_xmlToM21.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1290,7 +1290,7 @@ def testHiddenRestImpliedVoice(self):

self.assertEqual(len(MP.stream.voices), 2)
self.assertEqual(len(MP.stream.voices[0].elements), 1)
self.assertEqual(len(MP.stream.voices[1].elements), 2)
self.assertEqual(len(MP.stream.voices[1].elements), 1)
TimFelixBeyer marked this conversation as resolved.
Show resolved Hide resolved
self.assertEqual(MP.stream.voices[1].id, 'non-integer-value')

def testMultiDigitEnding(self):
Expand Down
94 changes: 25 additions & 69 deletions music21/musicxml/xmlToM21.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
from music21 import duration
from music21 import dynamics
from music21.common.enums import OrnamentDelay
from music21.common.numberTools import opFrac
from music21 import editorial
from music21 import environment
from music21 import exceptions21
Expand Down Expand Up @@ -866,6 +867,19 @@ def xmlRootToScore(self, mxScore, inputM21=None):
self.spannerBundle.remove(sp)

s.coreElementsChanged()
# Fill gaps with rests where needed
for m in s[stream.Measure]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why move this out of the MeasureParser? If it's just to have access to the md instance, we could just query for it in the measure parser (with getContextByClass, which is fast). To do that, we likely need to avoid coreInsert'ing it when inserting for the reason mentioned earlier.

Copy link
Contributor Author

@TimFelixBeyer TimFelixBeyer Dec 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a look - I moved it there since it didn't play nice with partwise musicxml files. I think this is because the rests inserted by makeRests don't have an entry in staffReference, and therefore can't be placed in the correct part until after the parts have been pulled apart.

One example is schoenberg op19.2. - testHiddenRests fails when keeping this inside the measure parser because a hole in voice 2 is filled with a half rest. this voice is supposed to be in the top staff, but the rest ends up in the bottom staff, where we end up with two voices instead of no voices (one with the newly inserted rest, and one with the actual data for the bottom staff).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to quickly resolve this in another way - lmk if you have any thoughts.

Copy link
Contributor Author

@TimFelixBeyer TimFelixBeyer Dec 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test wasn't failing before because we got lucky and this particular case happened in the last measure of the piece, where the incorrect rest was removed by removeEndForwardRest.

Copy link
Member

@jacobtylerwalls jacobtylerwalls Jan 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the ping. I wanted to see what you were talking about, so here's what I stubbed out:

diff --git a/music21/musicxml/test_xmlToM21.py b/music21/musicxml/test_xmlToM21.py
index ab1e99879..c8fe4421c 100644
--- a/music21/musicxml/test_xmlToM21.py
+++ b/music21/musicxml/test_xmlToM21.py
@@ -1547,4 +1547,4 @@ class Test(unittest.TestCase):
 
 if __name__ == '__main__':
     import music21
-    music21.mainTest(Test)
+    music21.mainTest('noDocTest', Test, runTest="testHiddenRests")
diff --git a/music21/musicxml/xmlToM21.py b/music21/musicxml/xmlToM21.py
index 0caa04f55..b385aeaab 100644
--- a/music21/musicxml/xmlToM21.py
+++ b/music21/musicxml/xmlToM21.py
@@ -821,7 +821,7 @@ class MusicXMLImporter(XMLParserBase):
             self.musicXmlVersion = mxVersion
 
         md = self.xmlMetadata(mxScore)
-        s.coreInsert(0, md)
+        s.insert(0, md)
 
         mxDefaults = mxScore.find('defaults')
         if mxDefaults is not None:
@@ -867,21 +867,7 @@ class MusicXMLImporter(XMLParserBase):
             self.spannerBundle.remove(sp)
 
         s.coreElementsChanged()
-        # Fill gaps with rests where needed
-        for m in s[stream.Measure]:
-            for v in m.voices:
-                if v:  # do not bother with empty voices
-                    # the musicDataMethods use insertCore, thus the voices need to run
-                    # coreElementsChanged
-                    v.coreElementsChanged()
-                    # Fill mid-measure gaps, and find end of measure gaps by ref to measure stream
-                    # https://github.com/cuthbertlab/music21/issues/444
-                    # but only when the score comes from Finale
-                    if any('Finale' in software for software in md.software):
-                        v.makeRests(refStreamOrTimeRange=m,
-                                    fillGaps=True,
-                                    inPlace=True,
-                                    hideRests=True)
+
         s.definesExplicitSystemBreaks = self.definesExplicitSystemBreaks
         s.definesExplicitPageBreaks = self.definesExplicitPageBreaks
         for p in s.parts:
@@ -2558,8 +2544,25 @@ class MeasureParser(SoundTagMixin, XMLParserBase):
                 if methName is not None:
                     meth = getattr(self, methName)
                     meth(mxObj)
+        md = self.parent.parent.stream[metadata.Metadata].first()
         for v in self.stream[stream.Voice]:
-            v.coreElementsChanged()
+            if v:  # do not bother with empty voices
+                # the musicDataMethods use insertCore, thus the voices need to run
+                # coreElementsChanged
+                v.coreElementsChanged()
+                # Fill mid-measure gaps, and find end of measure gaps by ref to measure stream
+                # https://github.com/cuthbertlab/music21/issues/444
+                # but only when the score comes from Finale
+                if md and any('Finale' in software for software in md.software):
+                    old_ids = {id(el) for el in v}
+                    v.makeRests(refStreamOrTimeRange=self.stream,
+                                fillGaps=True,
+                                inPlace=True,
+                                hideRests=True)
+                    for el in v:
+                        if id(el) not in old_ids:
+                            self.addToStaffReference(self.mxMeasure, el)
+
         self.stream.coreElementsChanged()
 
         if (self.restAndNoteCount['rest'] == 1

Now I get:

  File "/Users/jwalls/music21/music21/musicxml/test_xmlToM21.py", line 1282, in testHiddenRests
    self.assertIsInstance(hiddenRest, note.Rest)
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/unittest/case.py", line 1294, in assertIsInstance
    self.fail(self._formatMessage(msg, standardMsg))
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/unittest/case.py", line 715, in fail
    raise self.failureException(msg)
AssertionError: <music21.chord.Chord E-5 F#5 B-5 D6> is not an instance of <class 'music21.note.Rest'>

But when I look at this piece on IMSLP, that seems correct. No rests at the end of the file. So my next thought was to look at how the file is encoded in the version in the music21 corpus.

I have to run, but my intent is to get you an answer in the next few days, unless you already know the answer and can enlighten me :-)

Thanks for your persistence.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a look - I moved it there since it didn't play nice with partwise musicxml files.

Do you mean MusicXML files where some parts have more than one staff? Almost all MusicXML files are Partwise, and I don't think music21 even imports Timewise since I've never seen one in the wild.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I meant multi-staff parts, sorry. Basically anything with a <staff> tag that forces m21 to later use separateOutPartStaves to move something to a different staff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jacobtylerwalls i'm not 100% how addToStaffReference is supposed to work, but I'm pretty sure measure elements don't have a <staff> element assigned to them, only notes or rests do. Therefore the added rests will probably always be added to the "NO_STAFF_ASSIGNED" bucket. I think the right way to do this would be to find either the following or preceding note in the same voice and put the added rests in the same staff as that note.
Maybe separateOutPartStaves is smart enough to do that already?

for v in m.voices:
if v: # do not bother with empty voices
# the musicDataMethods use insertCore, thus the voices need to run
# coreElementsChanged
v.coreElementsChanged()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: if we decide to leave this here after all, this is probably not needed (but still think MeasureParser is better if I'm not missing something)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been a while since I've thought through this PR, but I think that this is correct.

# Fill mid-measure gaps, and find end of measure gaps by ref to measure stream
# https://github.com/cuthbertlab/music21/issues/444
v.makeRests(refStreamOrTimeRange=m,
fillGaps=True,
inPlace=True,
hideRests=True)
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved
s.definesExplicitSystemBreaks = self.definesExplicitSystemBreaks
s.definesExplicitPageBreaks = self.definesExplicitPageBreaks
for p in s.parts:
Expand Down Expand Up @@ -1760,32 +1774,8 @@ def parseMeasures(self):
for mxMeasure in self.mxPart.iterfind('measure'):
self.xmlMeasureToMeasure(mxMeasure)

self.removeEndForwardRest()
part.coreElementsChanged()

def removeEndForwardRest(self):
'''
If the last measure ended with a forward tag, as happens
in some pieces that end with incomplete measures,
and voices are not involved,
remove the rest there (for backwards compatibility, esp.
since bwv66.6 uses it)

* New in v7.
'''
if self.lastMeasureParser is None: # pragma: no cover
return # should not happen
lmp = self.lastMeasureParser
self.lastMeasureParser = None # clean memory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove this instance attribute now.


if lmp.endedWithForwardTag is None:
return
if lmp.useVoices is True:
return
endedForwardRest = lmp.endedWithForwardTag
if lmp.stream.recurse().notesAndRests.last() is endedForwardRest:
lmp.stream.remove(endedForwardRest, recurse=True)

def separateOutPartStaves(self) -> list[stream.PartStaff]:
'''
Take a `Part` with multiple staves and make them a set of `PartStaff` objects.
Expand Down Expand Up @@ -2233,7 +2223,7 @@ def adjustTimeAttributesFromMeasure(self, m: stream.Measure):
else:
self.lastMeasureWasShort = False

self.lastMeasureOffset += mOffsetShift
self.lastMeasureOffset = opFrac(self.lastMeasureOffset + mOffsetShift)

def applyMultiMeasureRest(self, r: note.Rest):
'''
Expand Down Expand Up @@ -2390,13 +2380,6 @@ def __init__(self,
# what is the offset in the measure of the current note position?
self.offsetMeasureNote: OffsetQL = 0.0

# keep track of the last rest that was added with a forward tag.
# there are many pieces that end with incomplete measures that
# older versions of Finale put a forward tag at the end, but this
# disguises the incomplete last measure. The PartParser will
# pick this up from the last measure.
self.endedWithForwardTag: note.Rest | None = None

@staticmethod
def getStaffNumber(mxObjectOrNumber) -> int:
'''
Expand Down Expand Up @@ -2553,19 +2536,8 @@ def parse(self):
if methName is not None:
meth = getattr(self, methName)
meth(mxObj)

if self.useVoices is True:
for v in self.stream.iter().voices:
if v: # do not bother with empty voices
# the musicDataMethods use insertCore, thus the voices need to run
# coreElementsChanged
v.coreElementsChanged()
# Fill mid-measure gaps, and find end of measure gaps by ref to measure stream
# https://github.com/cuthbertlab/music21/issues/444
v.makeRests(refStreamOrTimeRange=self.stream,
fillGaps=True,
inPlace=True,
hideRests=True)
for v in self.stream[stream.Voice]:
v.coreElementsChanged()
self.stream.coreElementsChanged()

if (self.restAndNoteCount['rest'] == 1
Expand All @@ -2588,18 +2560,16 @@ def xmlBackup(self, mxObj: ET.Element):
>>> mxBackup = EL('<backup><duration>100</duration></backup>')
>>> MP.xmlBackup(mxBackup)
>>> MP.offsetMeasureNote
0.9979
Fraction(9979, 10000)

>>> MP.xmlBackup(mxBackup)
>>> MP.offsetMeasureNote
0.0
'''
mxDuration = mxObj.find('duration')
if durationText := strippedText(mxDuration):
change = common.numberTools.opFrac(
float(durationText) / self.divisions
)
self.offsetMeasureNote -= change
change = opFrac(float(durationText) / self.divisions)
self.offsetMeasureNote = opFrac(self.offsetMeasureNote - change)
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved
# check for negative offsets produced by
# musicxml durations with float rounding issues
# https://github.com/cuthbertLab/music21/issues/971
Expand All @@ -2611,22 +2581,9 @@ def xmlForward(self, mxObj: ET.Element):
'''
mxDuration = mxObj.find('duration')
if durationText := strippedText(mxDuration):
change = common.numberTools.opFrac(
float(durationText) / self.divisions
)

# Create hidden rest (in other words, a spacer)
# old Finale documents close incomplete final measures with <forward>
# this will be removed afterward by removeEndForwardRest()
r = note.Rest(quarterLength=change)
r.style.hideObjectOnPrint = True
self.addToStaffReference(mxObj, r)
self.insertInMeasureOrVoice(mxObj, r)

change = opFrac(float(durationText) / self.divisions)
# Allow overfilled measures for now -- TODO(someday): warn?
self.offsetMeasureNote += change
# xmlToNote() sets None
self.endedWithForwardTag = r
self.offsetMeasureNote = opFrac(self.offsetMeasureNote + change)

def xmlPrint(self, mxPrint: ET.Element):
'''
Expand Down Expand Up @@ -2785,8 +2742,7 @@ def xmlToNote(self, mxNote: ET.Element) -> None:
self.nLast = c # update

# only increment Chords after completion
self.offsetMeasureNote += offsetIncrement
self.endedWithForwardTag = None
self.offsetMeasureNote = opFrac(self.offsetMeasureNote + offsetIncrement)

def xmlToChord(self, mxNoteList: list[ET.Element]) -> chord.ChordBase:
# noinspection PyShadowingNames
Expand Down Expand Up @@ -3578,7 +3534,7 @@ def xmlToDuration(self, mxNote, inputM21=None):
mxDuration = mxNote.find('duration')
if mxDuration is not None:
noteDivisions = float(mxDuration.text.strip())
qLen = common.numberTools.opFrac(noteDivisions / divisions)
qLen = opFrac(noteDivisions / divisions)
else:
qLen = 0.0

Expand Down Expand Up @@ -5510,7 +5466,7 @@ def parseAttributesTag(self, mxAttributes):
meth(mxSub)
# NOT to be done: directive -- deprecated since v2.
elif tag == 'divisions':
self.divisions = common.opFrac(float(mxSub.text))
self.divisions = opFrac(float(mxSub.text))
# TODO: musicxml4: for-part including part-clef
# TODO: instruments -- int if more than one instrument plays most of the time
# TODO: part-symbol
Expand Down