Enhance kern grace note importing, fix durations of partially notated chords #1706
Conversation
Now when reading a kern file, a grace note will have its unlinked duration type match the note's notated notation in the file. Also, specifying a grace note with 'qq' instead of just 'q' will omit the slash on the accidental. Also, when reading chords where only the first note of the chord specifies its duration, the non-first notes in the chord will inherit this duration. Similar to how a chord object uses the same literal duration object of the first note (not just a copy), these non-first notes that don't have their own specified duration also reuse the first note's duration object.
| eventChord = chord.Chord(chordNotes, beams=chordNotes[-1].beams) | ||
| eventChord.duration = chordNotes[0].duration | ||
|
|
||
| eventChord = chord.Chord(chordNotes, beams=chordNotes[-1].beams, # type: ignore |
There was a problem hiding this comment.
Can you explain the type: ignore here?
There was a problem hiding this comment.
I had to use type ignore where I couldn't reproduce the music21 type-checking locally and kept failing when pushing even though the types were right. It might have been an issue with things being set to None initially and then replaced with a different type according to some logic. I'll give this typing another try.
| def _hdStringToDuration(contents: str, | ||
| defaultDuration: duration.Duration|None = None) -> duration.Duration: |
There was a problem hiding this comment.
lint -- inconsistent indentation.
I still don't understand why a routine called XToDuration would take in a Duration object? This does not look right or make sense to me.
There was a problem hiding this comment.
Looking back at my pr description, it looks like I didn't copy over my previous example. The issue is that you occasionally get a humdrum chord in one voice that only notates the duration of the first note, as in:
8C E G
Humdrum at least allows for this and the Humdrum Verovio Viewer would parse these three notes all as eighth notes. Here's a minimal example showing this behavior. music21 currently parses the C as an eighth note but the E and the G are seen as not having a duration so they get the default duration of a duration object, which is a quarter note. A quarter note is a reasonable default duration to have in music21 but not in this circumstance. In this case it is helpful to be able to say "in humdrum chords, use the first note's duration as the default duration for the other notes in the chord if they don't specify one themselves". This is just like what music21 does for the chord object itself. The chord object not only has the same duration as the first note, it uses the same literal duration object. So this pr extends that same behavior to the other notes in the chord if they don't specify their own duration. So 8C E G would be a chord of three eighth notes but 8C 2E 2G would be a chord of one eighth note and two half notes. I encountered this issue in a Joplin rag kern file, but I don't know how common it is in general. Tests are also added to check this new behavior.
| if (qCount := contents.count('q')): | ||
| thisObject.getGrace(inPlace=True) | ||
| if qCount == 2: | ||
| thisObject.duration.slash = False # type: ignore |
There was a problem hiding this comment.
why type ignore -- this generally means that there's an error in the code logic. Please remove these.
There was a problem hiding this comment.
My guess is that a GraceDuration should be created separately and then typed in order to change its slash value. (It seems like an error on music21's logic in some way -- the slash is really on the GeneralNote not on the duration itself... but that's a larger PR)
There was a problem hiding this comment.
I think I tried to avoid creating a new grace note separately because that may have created an extra or even two extra duration objects. This seemed like a great application of the inPlace parameter. I'll double-check this typing issue as well.
| # Determine duration part first to avoid making an unused duration | ||
| thisDuration = _hdStringToDuration(contents, defaultDuration) |
There was a problem hiding this comment.
Is this line supposed to be something like:
if defaultDuration:
thisDuration = defaultDuration
else:
thisDuration = _hdStringToDuration(contents)
??? this is the part I'm most confused by.
There was a problem hiding this comment.
The idea is that the defaultDuration argument you pass will be None most of the time, but in the case of chords, you pass the duration object of the first note in the chord. Then, you check the second note's duration part of its string. If the second note has a duration part to its string, then you use that no matter what it is. It's only if the second note doesn't specify its own duration that we want to use the first note's duration. But using your if/else above, that would use the first note's duration (the defaultDuration argument) for all other notes in the chord no matter what. Maybe the problem is just a naming one, does fallbackDuration make more sense?
There was a problem hiding this comment.
Hi @mscuthbert does my explanation above clarify this? Would you be ok with calling it fallbackDuration? Passing the first note's duration to all other notes in that chord is key to fixing the bug explained in my "minimal example" given in a comment above. I'm on board with all your other comments, so if we can agree on this then I can update the pr.
This pr is a rework of a previous pr that I accidentally closed when resyncing my fork, sorry if that's confusing. I've addressed the comments that were made on that previous pr. This enhances kern grace note imports to allow "qq" to mean an unslashed grace note, and for the unlinked duration to take that of the written note if one is provided. This makes the unlinked duration of imported kern grace notes behave the same everywhere else in music21. Also, for kern chords with durations only specified on the first note in the chord ("8C E G"), non-first notes will now reuse that duration instead of defaulting to a quarter note duration. I also added 4 tests to the spineParser.py file to cover all of this new import behavior.
This pr also adds types to a couple functions in spineParser.py. I had a bit of a hard time with the mypy types because my local mypy setup doesn't give the same exact output as the CI test does. Please be aware that I had to use the comment
# type: ignore4 times in this file so I hope that's not an issue.Additionally, this pr separates duration processing into its own function, and decreases the overall import time for kern scores by ~12%. The speed up is primarily due to the following:
The previous version of this pr also included memoization and safe short-circuiting, but those were discarded in favor of simplicity. That earlier pr also replaced
str.count('x')withif x in strwhere possible but that has already been addressed by this pr.Looking quickly, this same basic idea of figuring out the duration part before creating a note could apply in tinyNotation, abc, and m21ToXml.py->GeneralObjectExporter.fromDuration/fromScale/fromDiatonicScale
And a similar idea, but figuring out pitch first, might apply in lots of places.