-
Notifications
You must be signed in to change notification settings - Fork 4
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
Fix missing pitches in neume ngrams + refactorings #913
Merged
Conversation
This file contains 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
Replaces underscores with hyphens to delimit words in multiword neume names. This disambiguates situation where we had been using underscores to delimit neume names and words within multiword neume names.
Neume types, mappings, and UI ordering are used across multiple modules. Their own helper module is a better location than within the MEI parsing helper module. Updates some type annotations and typing-related dependencies in the process.
…name hyphens refs: 9ff400c83a9feea0e78fccdde408d55c61d57199
Fixes the logic governing the creation of ngrams of neumes in the MEITokenizer class so that an ngram document containing multiple complete neumes contains all of the pitches contained within those neumes. Adds a new test to the MEITokenizer test class that ensures that the number of pitches in an ngram agrees with the number of pitches suggested by the neume names in the ngram.
Moves the TEST_MEI_FILES_PATH setting to settings.py rather than setting it separately for each test. Makes some changes to test set-up and break-down to remove the need to index MEI before each test.
dchiller
force-pushed
the
i910-compound-neumes
branch
from
September 23, 2024 18:37
9835c88
to
92a25b0
Compare
@lucasmarchd01 could you review? |
Yup sorry I missed this one! |
lucasmarchd01
approved these changes
Oct 4, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR makes a number of relatively minor changes.
admin.py
: adds missing type annotations, modifies field groups for the Manuscript admin page, modifies the listed fields for neume exemplarshelpers
file. Since these are used across the application, it made more sense to me to import them from a central file rather than having these lists and dictionaries spread across multiple filessettings.py
rather than separately in every test file where test mei files are usedIt also closes #910. Previously, neume ngrams (as opposed to pitch ngrams) did not include all the pitches contained in the final neume in the ngram. This PR adjusts the logic of the MEITokenizer (adding pitches to the ngram until the start of the n+1th neume is reached) to address this issue.