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.

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

New .sequence attribute on Voice to distinguish from .id and control display #915

Closed

Conversation

jacobtylerwalls
Copy link
Member

@jacobtylerwalls jacobtylerwalls commented Mar 13, 2021

Fixes #890

  • Standardizes voice numbering across notation routines and parsers
  • .sequence tracks the 1-indexed sequence in m21 containers and is updated regularly, whereas .id is reserved for user associations but also set by parser with literal value from parsed document
  • .flattenUnnecessaryVoices() now renumbers voices also
  • musicXML exporter reads from voice.sequence instead of voice.id (was possible to write out <voice>pianoRH-v2</voice>--legal, but nonstandard)
  • no changes to the VoiceIndex used by offsetMap() that is a literal 0-indexed count calculated on demand

FYI @napulen @gregchapman-dev, LMK if this meets your needs, or otherwise makes sense from a Humdrum perspective.

@gregchapman-dev
Copy link
Contributor

Happy to review the code when I have a moment. Not sure it will serve my needs as described, though. I need a way, in a Humdrum parser, to identify voices across parts (since a voice may cross into a different staff), so perhaps a combination of part/partstaff number and voice number would be better for a parser. Parsers need to be able to describe (in a standard way) any voice leading that is described in the format they are parsing.

@jacobtylerwalls
Copy link
Member Author

Thanks, that makes sense that the Humdrum parser would need such a feature. I suppose if I were attacking your problem, I would want a count of so-called Parts where each StaffGroup of PartStaffs counts once, labeled either with numbers or the actual part/group names, then add the voice number as a suffix, then store on .id. Then a quick test with voicesToParts(separateById=True) to see if you can take the resulting music21 stream with, say, three voices zagging around two PartStaffs and extract it into three parts.

I take it in Humdrum, voice leading information is continuous across measure boundaries? That information is completely lacking in musicxml.

@gregchapman-dev
Copy link
Contributor

Yes, humdrum voices cross measure boundaries (and can even temporarily cross part boundaries), which is great. On the other hand, unlike in music21, in humdrum you have to split spines (create new voices) if you have simultaneous notes that are not exactly the same duration, so the voice leading info you have might be fairly arbitrary (although if it crosses part boundaries, it's clear someone was actually trying to do real voice leading). I will play around with using voice.id in various ways to see what works best, and then I will come back with a proposal for (perhaps) yet another music21 voice attribute.

@jacobtylerwalls jacobtylerwalls force-pushed the voice-numbering branch 2 times, most recently from 5188cca to c9a4624 Compare March 29, 2021 18:34
@jacobtylerwalls jacobtylerwalls force-pushed the voice-numbering branch 2 times, most recently from 04ae095 to bc629f0 Compare April 1, 2021 14:27
@coveralls
Copy link

coveralls commented Apr 1, 2021

Coverage Status

Coverage decreased (-1.0e-05%) to 93.147% when pulling 68b0241 on jacobtylerwalls:voice-numbering into 79f2ea2 on cuthbertLab:master.

@jacobtylerwalls
Copy link
Member Author

jacobtylerwalls commented May 2, 2021

Coming back to this after Greg's mailing list post today. As part of determining whether the "analytical" voice designation will be documented as .id or something else--as opposed to the "display" voice numbering--it would be very welcome to see a community member contribute a sketch of how makeBeams could take that into account to make cross-staff beams.

I suggested .id just so that we don't incur the cost of rewriting voicesToParts().

@mscuthbert
Copy link
Member

Is number the right word for this? Sometimes voices are words like Soprano or Alto or Bass Trombone vs Tuba? Or Dux vs Comes.

I think this is the right track-- we need something a bit more specific than a group maybe--but I don't want to have to come back with something like .numberName in a few years.

@jacobtylerwalls
Copy link
Member Author

jacobtylerwalls commented May 3, 2021

"Sequence" would be another option for a name. I'll summarize where I think this is, not to be repetitive but because it clarified things for me and I don't want my thoughts to vaporize.

Before

Our musicXML exporter reads from .id when writing <voice> tags, unless the .id is an int, in which case we just renumber things using a "next free voice" counter.

Our musicXML importer puts <voice> tag numbers on .id, but we overwrite it in some scenarios (piano music), and other parsers do other things, and we also encourage users to manipulate .id, so it's hard to know what the roundtrip will be. The spec is liberal (any CDATA?) but I've mostly seen 1-indexed numbers for voice tags in files generated by other software. music21 was writing out things like "pianoLH-v1". I haven't tested a battery of musicXML readers to see what they do with that. I assume permissive software would just look for unique values.

What IS required in the spec is that the numbers increment (or a distinct value is used, I suppose) when going to the second PartStaff, because SHARING voice numbers across two part-staffs indicates cross-staff. In that sense "pianoRH-v2" is not helpful. If pianoRH-v2 is supposed to be beamed with the top voice of the LH, it needs to take the same <voice>, and vice versa. And all of these contexts reset completely from measure to measure, unlike Humdrum.

I do not think we should read from whatever field we give users to do their analysis (id/groups)? to write the voice tags. They'll get cross staff beaming music when they don't want it once they start associating voices together for analysis.

While we're here, I wanted to think about the effect on voicesToParts() and its separateById option. I can't imagine a use case for extracting and associating voices across a whole piece based on the voice numbers parsed from MusicXML, since the context unhelpfully resets per measure, but I could see it for "tenor", "dux", etc.. But for the musicxml exporter I was reluctant to read from any user analysis values, (how do I sort None, "2", "3", "Tenor", and "pianoLH-v2" and either bump or not bump the values on the subsequent staff?) Maybe musicxml import should set .sequence rather than .id? We would need a .freeze attribute or something so that flattenUnnecessaryVoices doesn't destroy the information (unless its force option is used).

Proposal

  • Original proposal: status quo, let MusicXML importer put tags on .id (New proposal: maybe it should do that but also set .sequence, if convertible to an int rather than use a counter? Makes sense given next point.)
  • MusicXML exporter reads from .sequence to write <voice> tags
  • Sequence attribute updated by flattenUnnecessaryVoices (but not if .freeze = True) during makeNotation calls. freeze=True set by musicXML importer but .freeze=False set by makeVoices() and notation routines. At Greg's option he can set freeze=True only if there is cross-staff beaming music to represent, otherwise False.
  • That means musicXML export can still renumber voices on the fly if makeNotation=True but not if False
  • 1-indexed to match most tags by other software
  • put the .sequence in the __repr__() of a Voice object along with .id
  • 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.

I probably won't return to this for a bit, but this seems like progress to get a proposal down. Greg, any thoughts? Set .id generally, but then also set .freeze = True on Voice objects when their .id should be interpreted to signify cross-staff?

Some guy named Myke asked about this in 2012: https://forums.makemusic.com/viewtopic.php?f=12&t=2392#p6457

@gregchapman-dev
Copy link
Contributor

gregchapman-dev commented May 3, 2021 via email

@jacobtylerwalls
Copy link
Member Author

I’m a little bummed that there isn’t a Part object that contains those two PartStaffs, but I understand that one more stream nesting level would be pretty painful.

I've worked on projects were we had such a layer between Score and Part. In music21 there was no real upside since joined PartStaff export was new in v6.5. Those assumptions could be revisited post-v6.5 but I imagine it would be very difficult to not break user assumptions this far into music21's life.

Maybe a “these PartStaffs are all one ‘part’” spanner?

We have a flavor of this, we have logic in partStaffExporter to determine which if any layout.StaffGroup spanners represent "PartStaffs that are all one part". Maybe we need to promote that to another module.

@jacobtylerwalls
Copy link
Member Author

My immediate reaction is that association of one Voice with another Voice (because they are the same lower-case ‘voice') should not be done via an existing field that is used for other things. I would prefer a new attribute (or maybe a new membership in a higher-level object, like a VoiceSpanner or something). That’s a reflexive response, not particularly thought through (it’s my “how do we add features without screwing existing customers and also without making the code unreadable” reflex).

Sure, but it's tricky, because we have existing functionality that is supposed to support this use case: voicesToParts(separateById=True). We've just never finished coding and testing the musicxml roundtrip, since as I mentioned in the last comment, there was no upside until joined part staff output was even possible. If we create new attributes, then what are the old ones for?

@jacobtylerwalls jacobtylerwalls force-pushed the voice-numbering branch 2 times, most recently from e51b49c to 858f31a Compare May 18, 2021 17:51
@jacobtylerwalls jacobtylerwalls changed the title WIP for .number attribute on Voice New .sequence attribute on Voice to distinguish from .id and control display May 18, 2021
@jacobtylerwalls jacobtylerwalls marked this pull request as ready for review May 18, 2021 17:55
@mscuthbert
Copy link
Member

What about using a .group? We have getElementsByGroup, etc.

@mscuthbert
Copy link
Member

Agreed though that something is needed.

@jacobtylerwalls
Copy link
Member Author

What about using a .group? We have getElementsByGroup, etc.

That's worth exploring.

v = stream.Voice()
v.id = vIndex # TODO: should use a separate voiceId or something in Voice.
v.id = vId # parsed from XML document
Copy link
Member Author

Choose a reason for hiding this comment

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

This one-line change could be its own PR discussed separately, maybe. Rather than "correct" whatever we get in the <voice> tags, just accept whatever we get.

@jacobtylerwalls
Copy link
Member Author

What about using a .group? We have getElementsByGroup, etc.

Hmm. Groups are strings, so if we want to query it deterministically later, that looks hairy, we're back to things like "piano-m10-v2" or similar, and what if the part is renamed?

I'm starting to think Greg's idea of a VoiceSpanner is the right approach. Leverage the infrastructure we already have for retrieving related objects that should display together.

It's also possible I can break this up into smaller PRs. This point seems noncontroversial and bite sized:

let MusicXML importer put content of <voice> tags on .id

@jacobtylerwalls jacobtylerwalls marked this pull request as draft October 21, 2021 17:25
@mscuthbert
Copy link
Member

VoiceSpanner is the way to go. Where'd I put that draft message I sent to the mailing list on other things that should be spanners.

@mscuthbert
Copy link
Member

Or .identifier for .sequence -- it's a term we've used elsewhere. When I see .sequence I think of any number of musical ideas (up a second; up a second... or Victimae Pascale Laudes) that don't connect here.

@mscuthbert
Copy link
Member

VoiceSpanner sounds really good, but what occurs to me now is that when we have, say, a slur spanner, if we have note K and note Q in it, and that's all, we can figure out that notes LMNOP are also in it, so no big deal if the spanner just has the outermost points or all the elements inside it. But with this VoiceSpanner, we would need to have all the Voice objects in it or they wouldn't be connected. Is that okay and something easily implemented?

@jacobtylerwalls
Copy link
Member Author

jacobtylerwalls commented Nov 30, 2021

VoiceSpanner sounds really good

Re-reading this thread I'm trying to see what VoiceSpanner is needed for, and I think it's only for cross-staff beaming. That was how Greg and I ended up here.

We already have id for associating voices together analytically ("this voice is also that voice"). We just need to track which ones display together as cross-staff, which isn't always the case even when they're "the same voice."

For Greg's humdrum converter I'm not certain any changes are needed; he can set .id; he would just hope to have a way to export good-looking musicxml (his original question).

I'm going to close this because when I can return I'll start with smaller PRs easier to digest. The discussion here was helpful! We can continue on #890 for any folks wanting to add thoughts.

@mscuthbert
Copy link
Member

I'd like to eventually get to .id being unique within a hierarchy, so VoiceSpanner or sequence is probably the best way to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[tracking] Improvements to voice numbering
4 participants