Humdrum: Retire defaultlist, GlobalComment bugs, GlobalRef m21obj?#1955
Merged
Conversation
defaultlist (a Perl-era helper that auto-extended/defaulted on out-of-bounds access) is retired here ahead of deprecating it. - EventCollection.events: dense, positional, one cell per spine -> list[SpineEvent|None] of length maxSpines (len() stays == maxSpines). - EventCollection.lastEvents: sparse (only '.' continuation cells) -> dict[int, SpineEvent], read via .get(). - createHumdrumSpines currentSpineList/newSpineList -> plain lists; currentSpineList is padded to maxSpines per line so spine-indexed reads still yield None for unassigned positions. Typing the spine lists surfaced latent assumptions the old Any hid: mergerActive is genuinely tri-state (bool|HumdrumSpine); exchangeActive only ever holds False-or-a-spine, so its sentinel becomes None (HumdrumSpine|None); and a spine-path token always has a spine from the first pass, now asserted. addGlobalEvent's HumdrumLine write is always overwritten by a placeholder SpineEvent, so events stays SpineEvent|None with a localized ignore. Verified note/part counts identical across 60 corpus kern files, including files with *v merges and *x exchanges. AI-assisted (Claude)
Now that humdrum no longer uses it, deprecate defaultlist via @deprecated on __init__. It was a transitional helper from music21's Perl port (read/assign past the end of a list -> default values); callers should use a plain list or dict. AI-assisted (Claude)
Replace `assert currentSpine is not None` with an explicit `if currentSpine is None: raise ValueError(...)` guarded by `# pragma: no cover` (the first pass guarantees a spine for any spine-path token, so the branch is unreachable in practice). AI-assisted (Claude)
addGlobalEvent filled EventCollection.events with the global HumdrumLine, but processEventForOneCell immediately overwrote every cell with a placeholder SpineEvent, and global events are inserted independently by insertGlobalEvents() reading self.eventList -- so nothing ever read those writes. Removing it also drops the type: ignore, making events: list[SpineEvent|None] unconditionally accurate. Verified note/part counts identical across 60 corpus kern files. AI-assisted (Claude)
…verload mergerActive previously overloaded one variable with three meanings: False (idle), a HumdrumSpine (the first spine's parent), or literal True (merge open but the first spine had no parent). The True-vs-spine distinction drove an `is not True` identity check and a bool|HumdrumSpine type that didn't parallel exchangeActive. Split it into mergerActive: bool (open flag) + mergerParentSpine: HumdrumSpine|None (the parent, None if none) -- which the code's own `# TODO: separate the two concepts` asked for. mergerParentSpine now parallels exchangeActive: HumdrumSpine|None. Behavior verified identical across 60 corpus kern files (incl. *v merges). AI-assisted (Claude)
Clearer name for the spine awaiting a partner to exchange with (or None). AI-assisted (Claude)
Covers the path that addGlobalEvent removal relied on: !! comments become GlobalComment objects in the stream, !!! references become metadata (composer/title), and a blank line is ignored without error. Previously only !!! -> metadata was tested (testMetadataRetrieved); GlobalComment -> stream had no coverage. AI-assisted (Claude)
insertGlobalEvents() ran before the parts were inserted into the Score, so a trailing !! comment (no following line -> appended at highestTime) was coreAppend'd to a still-empty Score and landed at offset 0.0 instead of the end of the music. Move the part-insertion ahead of insertGlobalEvents() so highestTime reflects the music. testGlobalEventsReachStream now asserts the leading comment is at 0.0 and the trailing comment at 2.0 (end of the two quarter notes). Note/part counts verified identical across 60 corpus kern files. AI-assisted (Claude)
GlobalReferences (known or unknown, anywhere incl. trailing) are routed to metadata -- known codes to proper fields, unknown codes to custom metadata -- and removed from the stream; none persist as stream objects. Assert that an unknown trailing !!!XYZ survives as custom metadata and that no GlobalReference is left in the stream. Only GlobalComments stay in the stream (and were the only globals affected by the offset fix). AI-assisted (Claude)
A GlobalReference never remains in the stream -- insertGlobalEvents put it there only so parseMetadata could pull it back out and delete it. So it doesn't need to be a Music21Object. Make GlobalReference a prebase.ProtoM21Object (like SpineEvent / HumdrumLine). insertGlobalEvents now collects references into self.globalReferences (still counting them so neighbouring comments keep their priority) and only inserts GlobalComments into the stream; parseMetadata reads self.globalReferences instead of pulling and removing GlobalReference objects from the stream. Metadata and GlobalComment offsets verified identical across 50 corpus kern files. AI-assisted (Claude)
…gs/comments Prose-only pass over docstrings and comments in __init__.py, spineParser.py, harmParser.py, and tests.py: capitalize sentence starts and proper nouns (Humdrum, Python, MusicXML, Recordare, Beethoven), end sentences with periods, and fix small grammar errors (e.g. "children spines" -> "child spines", "Italians" -> "Italian sixths", a stray "DynamicSpine" -> "DynamSpine" to match the real class). No code, doctest examples, expected output, or Humdrum jargon changed. (questions.py left untouched -- those are verbatim Humdrum quotes.) AI-assisted (Claude)
|
coverage: 93.134%. remained the same — retire_defaultlist into master |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
common.defaultlistwas a hack I made in the first couple of weeks of making music21 in Python. I had been working in Perl, where@a = []; $a[20] = 4;was totally acceptable to set$a[0]to$a[19]to None-type (undef) and used this constantly. So I put in defaultlist (originally PerlArray -- there was a PerlHash to go along with it). By the time I finished writing Humdrum parsing, I was ready to use actual Python lists. Nearly 20 years later, it's time for defaultlist to go.Surprisingly the two main places where defaultlist was used are best represented in opposite ways: self.events (spine events per line) are always filled, so setting to
[None] * maxSpineswas simple.lastEvents,however, was sparse, so best represented by a dict.GlobalComments were not be properly placed at the right point in the Stream. Fixed.
addGlobalEvent turned out to do nothing -- it would be immediately written over! :-) insertGlobalEvents was the actually functioning routine.
GlobalReference were made to be Music21Objects just so they could ride along with the Stream until the end and be removed. Now they are just ProtoM21Objects and they are all documented as affecting the whole Score.
AI-Assisted (Claude) -- PR written entirely by Myke.