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

Faster and more Robust Kern Import #344

Merged
merged 57 commits into from
May 22, 2024
Merged

Faster and more Robust Kern Import #344

merged 57 commits into from
May 22, 2024

Conversation

manoskary
Copy link
Member

@manoskary manoskary commented Jan 30, 2024

Faster and more Robust Kern Import

Updates

  • Using numpy.readtxt to ensure fast parsing and correct typesetting.
  • Two versions of import:
    1. Same number of voices from beginning to end
    2. Dynamic voices (spline splitting). This version prunes each voice starting from the leftmost side and removes them gradually from each line. Finally it replaces everything with a square matrix.
  • First creating partitura timed objects in a numpy array and deciding position on the timeline afterwards (See our paper on duration algebras)
  • Added some exceptions and corner cases for renaissance durations, a.o.

STATISTICS

  • Up to 3x faster for large corpora loading such as string quartets
  • Loads 98% of polish music corpus (~10k) scores.

NOT implemented yet

  • Articulation
  • Dynamics
  • Tempo markings
  • Page comments
  • Automatic Instrument grouping

spline-splitting version of kern creates conflicts with numpy version 1.24 when the delimiter of readtxt is `\n`. It has now been changed to `np.genfromtxt`
@codecov-commenter
Copy link

codecov-commenter commented Jan 30, 2024

Codecov Report

Attention: 98 lines in your changes are missing coverage. Please review.

Comparison is base (018f264) 84.96% compared to head (ef751af) 84.12%.
Report is 1 commits behind head on develop.

Files Patch % Lines
partitura/io/importkern_v2.py 90.84% 37 Missing ⚠️
partitura/score.py 42.85% 36 Missing ⚠️
partitura/io/exportkern.py 84.93% 25 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #344      +/-   ##
===========================================
- Coverage    84.96%   84.12%   -0.85%     
===========================================
  Files           82       83       +1     
  Lines        14236    14569     +333     
===========================================
+ Hits         12096    12256     +160     
- Misses        2140     2313     +173     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@manoskary manoskary linked an issue Jan 30, 2024 that may be closed by this pull request
@manoskary manoskary marked this pull request as draft February 13, 2024 12:55
@manoskary manoskary marked this pull request as ready for review April 24, 2024 08:48
partitura/__init__.py Outdated Show resolved Hide resolved
partitura/io/exportkern.py Show resolved Hide resolved
partitura/io/exportkern.py Outdated Show resolved Hide resolved
partitura/io/importkern.py Outdated Show resolved Hide resolved
tests/data/kern/voice_dublifications.krn Outdated Show resolved Hide resolved
@fosfrancesco
Copy link
Collaborator

fosfrancesco commented May 10, 2024

Ideally, it would be nice to have a lot of tests for the functions to fill rests since they can be a bit tricky. But since they are not the main topic of this branch, I would open another branch with only these tests, and eventual modifications to the functions.

@manoskary
Copy link
Member Author

Ideally, it would be nice to have a lot of tests for the functions to fill rests since they can be a bit tricky. But since they are not the main topic of this branch, I would open another branch with only these tests, and eventual modifications to the functions.

Thank you for the review. I agree this is particularly important. The kern import uses only measure wise rest infilling, meaning it doesn't happen for the entire piece but only for certain measures where the number of voices changes. I noticed that there are some lines that do not have coverage right now and I will address this in the tests. For the rest infilling, I agree that a separate PR with tests and checks would be probably better and clearer. Once the current PRs are closed I will update the tests.

tests/__init__.py Outdated Show resolved Hide resolved
@fosfrancesco fosfrancesco merged commit 214b244 into develop May 22, 2024
3 checks passed
Release 1.5.0 automation moved this from In progress to Done May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

Kern Parser - Parsing issues with stream separation Support for Renaissance Music Specific Score attributes.
3 participants