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

In kern allow non-eighth type & non-slashed grace notes, improve default duration of non-first chord notes, decrease import time by ~15% #1684

Closed
wants to merge 0 commits into from

Conversation

alexandermorgan
Copy link
Contributor

This PR has some very minor substantive changes with accompanying tests and several small optimizations for kern import

Substantive changes:

  1. Grace notes without slashes are now detected when two lowercase q's are present
  2. Grace notes apply their stated duration rather than always having eighth note representations. About these two grace note points, you can see in this example that humdrum is sensitive to the stated duration of a grace note, and also that the second q will omit the slash.
  3. In chords where only the first note has a stated duration, the other notes of the chord will take on the duration of the first note. The same exact duration object is used, similar to how chords already currently reuse the duration of their first note. Although standard humdrum syntax calls for a duration on each note of a chord, you can see here that when only the first note has a stated duration, other chords will use the same rather than defaulting to a quarter note.

Kern import optimizations (mostly in hdStringToNote)

  1. if str.count('x'): becomes if 'x' in str:
  2. Move duration parsing of notes to the beginning of hdStringToNote so that notes and rests can be instantiated with their correct duration from the beginning rather than being created and then having their duration replaced. This prevents an unused default duration from being created on each note and rest. Even better for chords, this pr makes them inherit the duration of their first note when the chord is created rather than right after creation, which means that instead of creating 1 unused duration per chord, now no new ones are created. I didn't check, but this super simple change for chords could be a quick and easy win for other import types if they construct chords in the same way.
  3. Combine the normal duration and rational duration regexes into one. Likewise for note name and accidental searches.
  4. Use walrus operator where possible to avoid duplicating operations
  5. Add short-circuit to stop processing a note/rest if all the characters have been accounted for after the duration and note name/accidental have been processed
  6. Comment out parsing that only results in pass statements
  7. Memoize the duration and note name regex searches inside of hdStringToNote. New durations, notes, and rests are still instantiated for each event.

@coveralls
Copy link

Coverage Status

coverage: 93.04% (+0.01%) from 93.03%
when pulling 8919f86 on alexandermorgan:master
into 0927e39 on cuthbertLab:master.

@alexandermorgan
Copy link
Contributor Author

Is there any input on this pr? It fixes two bugs for me when reading kern files so I would like to see it integrated if possible. I can also file those two bugs as issues if that would help. Thanks.

@mscuthbert
Copy link
Member

Hi Alex -- as I've mentioned on the list, I'm taking a hiatus from music21 except for paid support, unless I happen to have a bit of extra time. I'll try to get this when I can. Note that there's some conflict w/ the PR of Tim #1693 -- whichever gets merged first will need to have some edits on the other.

Copy link
Member

@mscuthbert mscuthbert left a comment

Choose a reason for hiding this comment

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

Hi -- sorry but I cannot accept this in the state it is in because I cannot follow the logic that it is supposed to use to speed up. The refactor adds much more complexity to an already overly complex routine.

@@ -2119,7 +2122,27 @@ def getAllOccurring(self):
return retEvents


def hdStringToNote(contents):
@lru_cache(maxsize=256)
def _noteMemos(contents):
Copy link
Member

Choose a reason for hiding this comment

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

new music21 routines require typing. Can you type the input and output of this routine?

Copy link
Member

Choose a reason for hiding this comment

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

Give some docs about what this does.

return (dur_regex, dotCount, matchedNote, step, accountedFor)


def hdStringToNote(contents, defaultDuration=None):
Copy link
Member

Choose a reason for hiding this comment

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

add typing at least to the new attribute (but it'd be kind to add typing to all)

raise HumdrumException(f'Could not parse {contents} for note information')
accountedFor += len(matchedNote.group(0))
step = matchedNote.group(1)[0].lower()
return (dur_regex, dotCount, matchedNote, step, accountedFor)
Copy link
Member

Choose a reason for hiding this comment

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

don't mix underscore and camelCase in internal variable names. Choose one or the other.

dur = defaultDuration
else:
if 'q' in contents:
dur = duration.Duration(.5, dots=dotCount)
Copy link
Member

Choose a reason for hiding this comment

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

style: 0.5 not .5

Comment on lines 2225 to 2228
dur_regex, dotCount, matchedNote, step, accountedFor = _noteMemos(contents)

if dur_regex:
foundNumber, foundRational = dur_regex.groups()
Copy link
Member

Choose a reason for hiding this comment

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

all of this takes an already very complex set of routines and makes them even more complex -- now with a set of nested if statements. Together with the rewriting of the count routines, I can't make heads or tails of what has changed. Please refactor into a routine that gets the duration from contents and one that gets the pitch from contents. I have no idea what "accountedFor" means or what a dur_regex is supposed to represent.

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.

None yet

3 participants