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

[TimFelixBeyer]

Fixes #1633
No longer create hidden rests while parsing forward tags in musicXML files, as this led to problems. If there are unfilled gaps, they will be filled later by makeRests anyways if required.
testHiddenRestImpliedVoice is modified since we no longer import
the empty forward tag as a trailing duration.
This also allows us to remove some code to deal with trailing rests.

[coveralls]

coverage: 93.023% (-0.003%) from 93.026%
when pulling 0a5a259 on TimFelixBeyer:TimFelixBeyer-patch-1
into 1dfb6d0 on cuthbertLab:master.

[jacobtylerwalls]

Hi @TimFelixBeyer thanks for submitting a patch. I want to suggest a different solution, which is not that we stop interpreting <forward> tags as hidden rests but that we just infer the correct voice number after a <backup> tag, i.e. that we make the solution from #1030 more robust. WDYT?

[TimFelixBeyer]

Hi @TimFelixBeyer thanks for submitting a patch. I want to suggest a different solution, which is not that we stop interpreting <forward> tags as hidden rests but that we just infer the correct voice number after a <backup> tag, i.e. that we make the solution from #1030 more robust. WDYT?

I completely agree with your logic about not 'inventing' elements where possible. In my mind it would therefore follow that we should also not interpret elements as rests (since they aren't rests).
Feel free to let me know your thoughts!
The ideal solution for me personally would be to not create hidden rests for forward tags & to also not run makeRests, since they both alter the score as it was saved to XML, as users could always run those functions later as well.

[jacobtylerwalls]

I guess without getting into the ontology of nothingness 😰, I think the reason for the status quo is that creating hidden rests in notation programs (at least Finale) will be exported as forward tags. And there is a slight difference between an explicit nothing marked as "do not show" versus an implicit nothing "document silent/incomplete".

[TimFelixBeyer]

I'm not super familiar with notation programs like finale, if forward tags are usually used to represent hidden rests, it totally makes sense to me to keep them. On the other hand if we decide that <forward> tags are always to be interpreted as hidden rests, I don't think there is a way for 'nothingness' to be represented in musicXML (from music21's perspective) at all, gaps will always have hidden rests (which is also what the makeRests call currently ensures). I don't really know whether this matters much/is maybe even intended.

[jacobtylerwalls]

But makeRests() is only called on export, so gaps/silence can still be represented on import or on export with makeNotation=False.

[TimFelixBeyer]

Maybe I’m missing something, but I don’t think this is accurate:

music21/music21/musicxml/xmlToM21.py

Line 2565 in fdb126c

v.makeRests(refStreamOrTimeRange=self.stream,

makeRests is called every time on import already, irrespective of the makeNotation keyword

[jacobtylerwalls]

Ah, but only in voices, not measures. I suppose the reason could be that incomplete voices are more likely to lead to display problems than incomplete measures but I suppose we could test out removing that call.

[TimFelixBeyer]

The call to makeRests was introduced in #444 so removing it would likely lead to the same problems described in that issue.
The only CI error I get is here, which is expected:

music21/music21/musicxml/test_xmlToM21.py

Line 1245 in f457a2b

def testHiddenRests(self):

I think it just depends on what the preference of the music21 maintainers is: how should <forward> tags be interpreted (as hidden rests, or as empty space)? My vote would be that even if programs like finale abuse the forward tag to represent hidden rests, that its not a faithful representation of the information in the musicXML format if we change forward tags to hidden rests.
Another alternative (most complex, but probably most accurate) would be to parse the tags differently depending on the <software> tag in the XML file.

[jacobtylerwalls]

I think a survey of other major musicxml writers would be helpful. What do MuseScore and Sibelius do? Dorico?

[Daniel63656]

In musicxml this is just "empty space". In fact, it is even less than that. Forward and Backward are just used to reposition a "cursor" so voices can also start mid-measure. If this is not handled, correct xml import is guaranteed to be wrong in a lot of cases as I illustrated in #1633

[jacobtylerwalls]

Sorry for the long delay. And thanks for the thoughtful responses. Agree we need to do something here, so I'll try to help suggest the least breaking change to get this in.

The call to makeRests was introduced in #444

Actually it was there before, but #444 just extended the "gap finding" to apply two additional arguments in order to find mid-measure and trailing rests. (makeRests defaults to only finding leading rests for some reason).

Another alternative (most complex, but probably most accurate) would be to parse the tags differently depending on the tag in the XML file.

Actually, that doesn't sound so bad. I bet we could just guard the fix for #444 under a check for the software tag, which has already been parsed (although somewhat inconveniently inserted with coreInsert(), so at this juncture has to be checked against _elements):

(Pdb) self.parent.parent.stream._elements[0].software
('Finale v26.3 for Mac')

Then #444 becomes something like:

if finale:
    v = makeRests(all four arguments)
else:
    v = makeRests(the two arguments before #444)

Then, @TimFelixBeyer you are right that removing the code in xmlForward to create leading hidden rests solves #1633, so I'm okay with that change and the related cleanups.

(Finally, the leading hidden rest that remains once we put #444 under a Finale guard doesn't cause problems with parsing and showing #1633, so I don't think we need to deal with that here to get this change in. If we want to remove it eventually, we can discuss it separately.)

Does that sound like a reasonable way to get #1633 fixed? LMK!

[TimFelixBeyer]

Thanks for your reply!

What do you think about this alternative:

if finale:
    v = makeRests(all four arguments)
else:
    pass

Or alternatively we just have two implementations for the forward tag, one for finale, which inserts hidden rests and one which doesn't do anything. Then we could skip all calls to makeRests during import.
I also took a look at MuseScore, there hidden rests are correctly exported as rests with print-object="no".

[jacobtylerwalls]

Alright, I'm on board. Getting rid of makeRests altogether on import for non-Finale sounds good.

[TimFelixBeyer]

@jacobtylerwalls It should now all be implemented as discussed and should fix #1633 and #1715.

I also added a test for finale and non-finale hidden-rest-on-<forward>-tag behavior.
Sorry for the messy commit history, I was using Github's Editor - feel free to squash them before merging.

Also had to include a minor mypy fix to common/enums.py to get the linting test to pass.

[jacobtylerwalls]

Great! Two little cleanups, and then in my opinion this will be g2g.

[jacobtylerwalls]

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.

[TimFelixBeyer]

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).

[TimFelixBeyer]

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

[TimFelixBeyer]

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.

[jacobtylerwalls]

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.

[mscuthbert]

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.

[TimFelixBeyer]

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.

[TimFelixBeyer]

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

[jacobtylerwalls]

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)

[mscuthbert]

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

[jacobtylerwalls]

I think we can remove this instance attribute now.

[mscuthbert]

Nice -- I suppose -- to know that there's not a future version of Finale on its way which might break this.

[TimFelixBeyer]

Any thoughts on my comment?

[mscuthbert]

btw -- I am mostly staying out of this for now, because Jacob is really the expert on the <forward> <backward> and <staff> parsing -- Chris Ariza wrote the original code, the refactoring I did didn't touch this code, and Jacob did a ton of work on it later, so it's one of the (not many) parts of music21 that I'm not an expert on.