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

mei measure number is now an integer, and. Measure.name is the string… #310

Merged
merged 3 commits into from
Aug 31, 2023

Conversation

fosfrancesco
Copy link
Member

Fixed a problem with Measure elements in MEI loading.
Now measure has 2 attributes:

  • number: an integer, from 0, which increase at each measure
  • name: the string how is encoded by the person writing the score (it may start from 1 or have strings inside)

@fosfrancesco fosfrancesco added the bug Something isn't working label Aug 23, 2023
@fosfrancesco fosfrancesco added this to In progress in Release 1.4.0 via automation Aug 23, 2023
@fosfrancesco fosfrancesco linked an issue Aug 23, 2023 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented Aug 23, 2023

Codecov Report

Patch coverage: 73.07% and project coverage change: +0.01% 🎉

Comparison is base (5fe996d) 85.13% compared to head (cc28af0) 85.14%.
Report is 1 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #310      +/-   ##
===========================================
+ Coverage    85.13%   85.14%   +0.01%     
===========================================
  Files           80       80              
  Lines        13663    13673      +10     
===========================================
+ Hits         11632    11642      +10     
  Misses        2031     2031              
Files Changed Coverage Δ
partitura/io/musescore.py 27.47% <0.00%> (ø)
partitura/utils/music.py 74.20% <ø> (ø)
partitura/io/importmei.py 92.19% <84.61%> (+0.03%) ⬆️
tests/test_mei.py 99.52% <100.00%> (+0.01%) ⬆️

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

@gregchapman-dev
Copy link

Can I recommend the name "index" instead of "number"?

@fosfrancesco
Copy link
Member Author

I think it is a good proposal to avoid confusion.

But this will need to be addressed in a separate pull request since we will need to update it also for all the other parsers, and some functions like measure_number_map. I will open an issue for remembering and for discussion.

The goal of this pull request is to have the same behaviour as the musicxml parser.

Copy link
Collaborator

@huispaty huispaty left a comment

Choose a reason for hiding this comment

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

The suggested change works and corresponds to how we parse measures in musicxml.
The Measure object has the number and name attributes. Currently, the number attribute represents a running count of measures (tags), irrespective of their regularity or volta endings - thus it starts at 1 and continuously increments. However, we could also interpret number more akin to an index (as suggested here) - and rename it to index accordingly. Let me know what you prefer.

@fosfrancesco
Copy link
Member Author

I found a bug with volta measures and corrected it. I would need new approval. Thank you!

@fosfrancesco fosfrancesco merged commit b7c1b35 into develop Aug 31, 2023
3 checks passed
Release 1.4.0 automation moved this from In progress to Done Aug 31, 2023
@huispaty huispaty mentioned this pull request Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Measure number are strings for MEI files
4 participants