Fix issue #1335: voice numbers written in MusicXML must be unique…

[gregchapman-dev]

…within the StaffGroup.

[coveralls]

Coverage increased (+0.008%) to 93.066% when pulling da3ba8c on gregchapman-dev:gregc/voiceNumbers into 9744786 on cuthbertLab:master.

[mscuthbert]

I think that this looks good. @jacobtylerwalls did a lot of work on the PartStaffExporter, so I'd like him to weigh in before merging. Please remove newly added code that is commented out. thanks!

[jacobtylerwalls]

Thanks for picking this up. Can you add a test showing we don't destroy intentional cross-staff beaming? If you parse in a musicxml document with cross-staff beams (voice 1 on both hands) then I think we still want the same voice number on export, not voices n and n+1. No?

I have a feeling you might reply that your module uses voice numbers in a different way than musicxml, and you'd prefer not to delete voices in measures that don't need display voices and prefer not to make your voice numbers distinct since you wish to signal analytical unity. But until we create a way to divorce "display" voices from "analytical" voices in #890, I don't see how. I probably need to close #890 and create a more approachable summary after the discussion on #915, but I just didn't get to it yet.

---

From #915:

Cross-staff music: PartStaffExporter currently bumps all voice numbers in the subsequent part staff to ensure distinct voice numbers, i.e. to ensure the 99% use case of no cross staff. When we WANT cross-staff, PartStaffExporter needs to get that knowledge from somewhere. Something like, per measure, if two music21 voices share the same .id/.group and .retain = True, if they've not already been given the same .sequence, give them the same .sequence and renumber everything else around it. This is a bit like the implementation in flattenUnnecessaryVoices, above, but different insofar as it would need to be calculated per measure over all voices from all PartStaffs in a joinable group.

Nasty problem. See also w3c/mnx#183.

[jacobtylerwalls]

Thanks for pushing forward -- let me know what you think about my questions below.

[mscuthbert]

I'm a little concerned that we're going further in the direction of manipulating XML that we have generated (which I didn't like in the initial PartStaffExporter proposal) and away from changing Python objects such as Voices etc. Can we do more preprocessing before XML is written (since this is already a deepcopy) to keep the complexity down?

[gregchapman-dev]

OK, I'm back to looking at this "voice numbers" PR.

I will definitely add support for unnumbered measures, and I'll try to manipulate the voice.id rather than the generated XML.

[gregchapman-dev]

I like this a lot better. I scan the score to compute the voiceNumberOffset for each PartExporter, then do the parse. Since I'm not doing measure-by-measure offsets (I use the same offset in every Measure in a Part), the measure numbers don't matter at all. And the partExp.voiceNumberOffset is used to offset the voice.id, avoiding further manipulation of generated XML.

[jacobtylerwalls]

Thanks, this is looking good. I agree with Myke that in 2022 if I were doing the partStaffExporter again I would want to do it all without post-processing any xml.

My main question is just why we are tracking the voice number offsets at the part level? I would have thought they need to be tracked at the measure level.

TREBLE
m1.---------- m2.
v1, v2--------no-voices
BASS
m1.----------m2.
v1, v2.-------v1, v2

Wouldn't this PR now unnecessarily turn the bass clef in measure 2 into voices 3 and 4 (instead of leaving it as 1 and 2?) This would be worth testing in some musicxml readers -- especially when the voice numbers end up above 4. And a unit test with multiple measures would be good.

[gregchapman-dev]

My main question is just why we are tracking the voice number offsets at the part level? I would have thought they need to be tracked at the measure level.

For simplicity, because I found I didn’t care so much about the case below (using higher voice numbers than necessary in some measures). But since you do care, I will use your cool idea about zip_measures, and track voice number offsets at the measure level...

Wouldn't this PR now unnecessarily turn the bass clef in measure 2 into voices 3 and 4 (instead of leaving it as 1 and 2?) This would be worth testing in some musicxml readers -- especially when the voice numbers end up above 4. And a unit test with multiple measures would be good.

I will add such a test.

[jacobtylerwalls]

Great. Yeah it's not so much that I care at a philosophical level, just that of all the musicxml readers out there, some of them (MuseScore?) I would expect to do funny things when the voice numbers start getting high.

[gregchapman-dev]

OK, ready for another review.

[jacobtylerwalls]

Thanks for the changes, this is looking great.

[gregchapman-dev]

Wow, flake8 got a lot pickier throughout music21. Hope that doesn't block me from getting this in.

[jacobtylerwalls]

Thanks, Greg!

[jacobtylerwalls]

Wow, flake8 got a lot pickier throughout music21. Hope that doesn't block me from getting this in.

See #1347, you could merge master now.

[mscuthbert]

Loving the huge amounts of tests and attention to detail!

My comments are mostly on trying to simplify the logic and remove the zipMeasures and other helper functions that seem to largely duplicate functionality that's already core to music21, and make it so that it's much easier to see still what ScoreExporter does in the case where there aren't staffgroups, PartStaffs etc. Very close!

[mscuthbert]

this is a bit of an O(n^2) approach. It's probably fine since n=num parts, and likely to be very fast, but in general there are ways to do this in O(n) time.

[mscuthbert]

nevermind, see above.

[mscuthbert]

this title should be more explicit: renumberVoicesWithinPartGroups, or something like that. Then I think for simplicity, it would be better to call two functions -- setPartExporterStaffGroups() before this is called, and then this function, which should itself call a helper function for every partExporter with a staff group. Doing more than one thing per function leads to added complexity.

Once you have a partExporter with the spannedElements, there's no need for all the simultaneous measure finding, just do something like this:

staffGroupScore = Score(partExp.staffGroup.getSpannedElements())
for measureStack in OffsetIterator(staffGroupScore).getElementsByClass(Measure):
     maxVoiceId = 0
     for m in measureStack:
           for v in m[Voice]:
                 ...  # do something

I think that would make the logic much simpler.

[mscuthbert]

And then we can just renumber voices here and not make the PartExporter have to keep track of them.

[jacobtylerwalls]

Wow, I can't believe I didn't think of OffsetIterator!

[gregchapman-dev]

I'm doing the following:

for measureStack in OffsetIterator(staffGroupScore).getElementsByClass(stream.Measure):

and I never get any measureStacks. Does OffsetIterator not recurse? The staffGroupScore I'm testing with contains 2 PartStaff objects, each of which has a Measure.

[gregchapman-dev]

Made it work by constructing a stream containing all the Measures. I'm guessing there might be a more efficient iterator-y way, but OffsetIterator really seemed to want (1) a Stream, not an Iterator, with (2) all the Measures at the top-level of that Stream.

[gregchapman-dev]

I believe this is complete, and ready for review now.

[mscuthbert]

I'm doing the following:

for measureStack in OffsetIterator(staffGroupScore).getElementsByClass(stream.Measure):

and I never get any measureStacks. Does OffsetIterator not recurse? The staffGroupScore I'm testing with contains 2 PartStaff objects, each of which has a Measure.

AH. it might not. SHOOT.

I'm working on new Iterator types right now, and I think I've gotten down how to make them work more simply. So I should be able to add a recurse=True to OffsetIterator instead. Okay, no requirement to implement it here.

[mscuthbert]

Everything looks great! Congrats!