Fix #1633

[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.032% (-0.004%) from 93.036% when pulling 273e9b1 on TimFelixBeyer:TimFelixBeyer-patch-1 into 8baa273 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?

[jacobtylerwalls]

I think this is too presumptive on the part of the parser. We should be interpolating/inventing as few elements as possible that are not in the document.

[TimFelixBeyer]

I agree!
I just put the makeRests there because previously the parser also ran makeRests (this block is basically just copy pasted from before), but I'd be more than happy to remove it.

[jacobtylerwalls]

This is a useful change that I'd be glad to accept in isolation.

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