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.

Sign up for GitHub

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

Enhance kern grace note importing, fix durations of partially notated chords #1706

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Enhance kern grace note importing, fix durations of partially notated chords #1706

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3b345f1
Improve grace note and chord reading for kern
alexandermorgan Apr 30, 2024
b290d32
Address mypy typing errors
alexandermorgan May 1, 2024
3eace65
Address more mypy issues
alexandermorgan May 1, 2024
91e2a65
Handle more mypy errors
alexandermorgan May 1, 2024
f503cb6
Resolve mypy errors in spineParser.py
alexandermorgan May 1, 2024
020758f
Create Duration before notes/rests to avoid placeholder durations
alexandermorgan May 1, 2024
875e412
Remove whitespace and fix mypy errors
alexandermorgan May 1, 2024
5027404
Fix docstring test
alexandermorgan May 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
290 changes: 168 additions & 122 deletions music21/humdrum/spineParser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
from music21 import meter
from music21 import metadata
from music21 import roman
from music21 import pitch
from music21 import prebase
from music21 import stream
from music21 import tempo
Expand Down Expand Up @@ -1295,7 +1296,7 @@ def __init__(self, spineId: int = 0, eventList=None, streamClass=stream.Stream):
super().__init__(spineId, eventList, streamClass)
self.lastContainer = None
self.inTuplet = None
self.lastNote = None
self.lastNote: t.Optional[note.Note | note.Rest | chord.Chord] = None
self.currentBeamNumbers = 0
self.currentTupletDuration = 0.0
self.desiredTupletDuration = 0.0
Expand Down Expand Up @@ -1363,7 +1364,7 @@ def processNoteEvent(self, eventC):
self.lastNote = eventNote
return eventNote

def processChordEvent(self, eventC):
def processChordEvent(self, eventC: str) -> chord.Chord:
'''
Process a single chord event

Expand All @@ -1372,12 +1373,14 @@ def processChordEvent(self, eventC):
# multipleNotes
notesToProcess = eventC.split()
chordNotes = []
for noteToProcess in notesToProcess:
thisNote = hdStringToNote(noteToProcess)
defaultDuration = None # used for non-first notes without a duration
for i, noteToProcess in enumerate(notesToProcess):
thisNote = hdStringToNote(noteToProcess, defaultDuration)
if i == 0:
defaultDuration = thisNote.duration
chordNotes.append(thisNote)
eventChord = chord.Chord(chordNotes, beams=chordNotes[-1].beams)
eventChord.duration = chordNotes[0].duration

eventChord = chord.Chord(chordNotes, beams=chordNotes[-1].beams, # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain the type: ignore here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

duration=defaultDuration)
self.setBeamsForNote(eventChord)
self.setTupletTypeForNote(eventChord)
self.lastNote = eventChord
Expand Down Expand Up @@ -2118,7 +2121,85 @@ def getAllOccurring(self):
return retEvents


def hdStringToNote(contents):
def _hdStringToDuration(contents: str,
defaultDuration: duration.Duration|None = None) -> duration.Duration:
Comment on lines +2124 to +2125
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

'''
returns a :class:`~music21.duration.Duration` matching the current
SpineEvent.

This is used internally by hdStringToNote to figure out the duration part
of a humdrum note or rest in a kern spine.
'''
# 3.2.7 Duration +
# 3.2.8 N-Tuplets
dotCount = contents.count('.')
durationRegex = re.search(r'(\d+)%?(\d+)?', contents)
if durationRegex:
foundNumber, foundRational = durationRegex.groups()

if foundRational:
durationFirst = int(foundNumber)
durationSecond = float(foundRational)
quarterLength = 4 * durationSecond / durationFirst
thisDuration = duration.Duration(quarterLength, dots=dotCount)

elif foundNumber:
durationType = int(foundNumber)
if durationType == 0:
if foundNumber == '000':
# for larger values, see https://extras.humdrum.org/man/rscale/
_type = 'maxima'
elif foundNumber == '00':
# for larger values, see https://extras.humdrum.org/man/rscale/
_type = 'longa'
else:
_type = 'breve'
thisDuration = duration.Duration(type=_type, dots=dotCount)
elif durationType in duration.typeFromNumDict:
_type = duration.typeFromNumDict[durationType]
thisDuration = duration.Duration(type=_type, dots=dotCount)
else:
dT = durationType + 0.0
(unused_remainder, exponents) = math.modf(math.log2(dT))
baseValue = 2 ** exponents
baseValueInt = int(baseValue)
_type = duration.typeFromNumDict[baseValueInt]
thisDuration = duration.Duration(type=_type)
newTup = duration.Tuplet()
newTup.durationActual = duration.durationTupleFromTypeDots(_type, 0)
newTup.durationNormal = duration.durationTupleFromTypeDots(_type, 0)

gcd = math.gcd(durationType, baseValueInt)
newTup.numberNotesActual = int(dT / gcd)
newTup.numberNotesNormal = int(baseValue / gcd)

# The Josquin Research Project uses an incorrect definition of
# humdrum tuplets that breaks normal usage. TODO: Refactor adding a Flavor = 'JRP'
# code that uses this other method...
JRP = flavors['JRP']
if JRP is False and dotCount:
newTup.durationNormal = duration.durationTupleFromTypeDots(
newTup.durationNormal.type, dotCount) # type: ignore

thisDuration.appendTuplet(newTup)
if JRP is True and dotCount:
thisDuration.dots = dotCount
# call Duration.TupletFixer after to correct this.

elif defaultDuration is not None:
thisDuration = defaultDuration

else: # no duration string or default duration given
if 'q' in contents:
thisDuration = duration.Duration(0.5, dots=dotCount)
else:
thisDuration = duration.Duration(1, dots=dotCount)

return thisDuration


def hdStringToNote(contents: str,
defaultDuration: duration.Duration|None = None) -> note.Note | note.Rest:
'''
returns a :class:`~music21.note.Note` (or Rest or Unpitched, etc.)
matching the current SpineEvent.
Expand Down Expand Up @@ -2201,49 +2282,62 @@ def hdStringToNote(contents):

# http://www.lib.virginia.edu/artsandmedia/dmmc/Music/Humdrum/kern_hlp.html#kern

# 3.2.1 Pitches and 3.3 Rests

matchedNote = re.search('([a-gA-G]+)', contents)
thisObject = None
# Determine duration part first to avoid making an unused duration
thisDuration = _hdStringToDuration(contents, defaultDuration)
Comment on lines +2285 to +2286
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


# 3.2.1 Pitches and 3.3 Rests
thisObject: t.Union[note.Note|note.Rest]
# Detect rests first, because rests can contain manual positioning information,
# which is also detected by the `matchedNote` variable above.
if 'r' in contents:
thisObject = note.Rest()
thisObject = note.Rest(duration=thisDuration)

elif matchedNote:
elif (matchedNote := re.search('([a-gA-G]+)([n#-]*)', contents)):
kernNoteName = matchedNote.group(1)
step = kernNoteName[0].lower()
if step == kernNoteName[0]: # middle C or higher
octave = 3 + len(kernNoteName)
else: # below middle C
octave = 4 - len(kernNoteName)
thisObject = note.Note(octave=octave)
thisObject.step = step

accid = matchedNote.group(2)
if accid:
_pitch = pitch.Pitch(step, octave=octave, accidental=accid)
else:
_pitch = pitch.Pitch(step, octave=octave)

thisObject = note.Note(_pitch, duration=thisDuration)

# Handle note-specific attributes
# 3.2.6 Stem Directions
if '/' in contents:
thisObject.stemDirection = 'up'
elif '\\' in contents:
thisObject.stemDirection = 'down'
# 3.2.10 Beaming
# TODO: Support really complex beams
for i in range(contents.count('L')):
thisObject.beams.append('start')
for i in range(contents.count('J')):
thisObject.beams.append('stop')
for i in range(contents.count('k')):
thisObject.beams.append('partial', 'right')
for i in range(contents.count('K')):
thisObject.beams.append('partial', 'right')

else:
raise HumdrumException(f'Could not parse {contents} for note information')

matchedSharp = re.search(r'(#+)', contents)
matchedFlat = re.search(r'(-+)', contents)

if matchedSharp:
thisObject.pitch.accidental = matchedSharp.group(0)
elif matchedFlat:
thisObject.pitch.accidental = matchedFlat.group(0)
elif 'n' in contents:
thisObject.pitch.accidental = 'n'

# 3.2.2 -- Slurs, Ties, Phrases
# TODO: add music21 phrase information
for i in range(contents.count('{')):
pass # phraseMark start
for i in range(contents.count('}')):
pass # phraseMark end
for i in range(contents.count('(')):
pass # slur start
for i in range(contents.count(')')):
pass # slur end
# for i in range(contents.count('{')):
# pass # phraseMark start
# for i in range(contents.count('}')):
# pass # phraseMark end
# for i in range(contents.count('(')):
# pass # slur start
# for i in range(contents.count(')')):
# pass # slur end
if '[' in contents:
thisObject.tie = tie.Tie('start')
elif ']' in contents:
Expand Down Expand Up @@ -2275,10 +2369,10 @@ def hdStringToNote(contents):
t1.connectedToPrevious = True # true by default, but explicitly
thisObject.expressions.append(t1)

if ':' in contents:
# TODO: deal with arpeggiation -- should have been in a
# chord structure
pass
# if ':' in contents:
# # TODO: deal with arpeggiation -- should have been in a
# # chord structure
# pass

if 'O' in contents:
thisObject.expressions.append(expressions.Ornament())
Expand Down Expand Up @@ -2306,93 +2400,18 @@ def hdStringToNote(contents):
elif 'u' in contents:
thisObject.articulations.append(articulations.DownBow())

# 3.2.6 Stem Directions
if '/' in contents:
thisObject.stemDirection = 'up'
elif '\\' in contents:
thisObject.stemDirection = 'down'


# 3.2.7 Duration +
# 3.2.8 N-Tuplets

# TODO: SPEEDUP -- only search for rational after foundNumber...
foundRational = re.search(r'(\d+)%(\d+)', contents)
foundNumber = re.search(r'(\d+)', contents)
if foundRational:
durationFirst = int(foundRational.group(1))
durationSecond = float(foundRational.group(2))
thisObject.duration.quarterLength = 4 * durationSecond / durationFirst
if '.' in contents:
thisObject.duration.dots = contents.count('.')

elif foundNumber:
durationType = int(foundNumber.group(1))
if durationType == 0:
durationString = foundNumber.group(1)
if durationString == '000':
# for larger values, see https://extras.humdrum.org/man/rscale/
thisObject.duration.type = 'maxima'
elif durationString == '00':
# for larger values, see https://extras.humdrum.org/man/rscale/
thisObject.duration.type = 'longa'
else:
thisObject.duration.type = 'breve'
if '.' in contents:
thisObject.duration.dots = contents.count('.')
elif durationType in duration.typeFromNumDict:
thisObject.duration.type = duration.typeFromNumDict[durationType]
if '.' in contents:
thisObject.duration.dots = contents.count('.')
else:
dT = int(durationType) + 0.0
(unused_remainder, exponents) = math.modf(math.log2(dT))
baseValue = 2 ** exponents
thisObject.duration.type = duration.typeFromNumDict[int(baseValue)]
newTup = duration.Tuplet()
newTup.durationActual = duration.durationTupleFromTypeDots(thisObject.duration.type, 0)
newTup.durationNormal = duration.durationTupleFromTypeDots(thisObject.duration.type, 0)

gcd = math.gcd(int(dT), int(baseValue))
newTup.numberNotesActual = int(dT / gcd)
newTup.numberNotesNormal = int(float(baseValue) / gcd)

# The Josquin Research Project uses an incorrect definition of
# humdrum tuplets that breaks normal usage. TODO: Refactor adding a Flavor = 'JRP'
# code that uses this other method...
JRP = flavors['JRP']
if JRP is False and '.' in contents:
newTup.durationNormal = duration.durationTupleFromTypeDots(
newTup.durationNormal.type, contents.count('.'))

thisObject.duration.appendTuplet(newTup)
if JRP is True and '.' in contents:
thisObject.duration.dots = contents.count('.')
# call Duration.TupletFixer after to correct this.

# 3.2.9 Grace Notes and Groupettos
if 'q' in contents:
thisObject = thisObject.getGrace()
thisObject.duration.type = 'eighth'
if (qCount := contents.count('q')):
thisObject.getGrace(inPlace=True)
if qCount == 2:
thisObject.duration.slash = False # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

why type ignore -- this generally means that there's an error in the code logic. Please remove these.

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

elif 'Q' in contents:
thisObject = thisObject.getGrace()
thisObject.duration.slash = False
thisObject.duration.type = 'eighth'
thisObject.getGrace(inPlace=True)
thisObject.duration.slash = False # type: ignore
elif 'P' in contents:
thisObject = thisObject.getGrace(appoggiatura=True)
elif 'p' in contents:
pass # end appoggiatura duration -- not needed in music21...

# 3.2.10 Beaming
# TODO: Support really complex beams
for i in range(contents.count('L')):
thisObject.beams.append('start')
for i in range(contents.count('J')):
thisObject.beams.append('stop')
for i in range(contents.count('k')):
thisObject.beams.append('partial', 'right')
for i in range(contents.count('K')):
thisObject.beams.append('partial', 'right')
thisObject.getGrace(appoggiatura=True, inPlace=True)
# elif 'p' in contents:
# pass # end appoggiatura duration -- not needed in music21...

return thisObject

Expand Down Expand Up @@ -2899,6 +2918,33 @@ def testSingleNote(self):
self.assertEqual(b.duration.dots, 0)
self.assertEqual(b.duration.tuplets[0].durationNormal.dots, 2)

def testGraceNote(self):
ks = KernSpine()
# noinspection SpellCheckingInspection
a = ks.processNoteEvent('4Cq')
self.assertEqual(a.duration.type, 'quarter')
self.assertEqual(a.duration.slash, True)

def testGraceNote2(self):
ks = KernSpine()
# noinspection SpellCheckingInspection
a = ks.processNoteEvent('16Cqq')
self.assertEqual(a.duration.type, '16th')
self.assertEqual(a.duration.slash, False)

def testChord(self):
ks = KernSpine()
# noinspection SpellCheckingInspection
c = ks.processChordEvent('8C 8E')
self.assertEqual(len(c.notes), 2)
self.assertEqual(c.notes[0].duration, c.duration)

def testChord2(self):
# noinspection SpellCheckingInspection
ks = KernSpine()
c = ks.processChordEvent('8C E')
self.assertEqual(c.notes[0].duration, c.notes[1].duration)

def testMeasureBoundaries(self):
m0 = stream.Measure()
m1 = hdStringToMeasure('=29a;:|:', m0)
Expand Down
Loading