Minor bug fixes in note features and Uniformity in Loading Score objects #207
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #207 +/- ##
===========================================
+ Coverage 85.00% 85.22% +0.21%
===========================================
Files 72 75 +3
Lines 12653 12942 +289
===========================================
+ Hits 10756 11030 +274
- Misses 1897 1912 +15
☔ View full report in Codecov by Sentry. |
Should add some tests. |
partitura/io/importmatch.py
Outdated
| continue | ||
| # continue introduces inconsistency when save_match | ||
| # continue | ||
| pass |
There was a problem hiding this comment.
Should we keep this open for a less hacky fix?
There was a problem hiding this comment.
This needs a proper fix, even after the PR is merged.
This was not so easy to do because it depends on the numerator and denominator of the time signature. I think this feature should be changed completely, maybe partially replaced by the metrical position features of the note array. |
|
Also, add link fix for issue #217 |
# Conflicts: # partitura/musicanalysis/performance_codec.py
partitura/io/importmatch.py
Outdated
| # Check if the note is tied and if so, add the tie information | ||
| if is_tied: | ||
| for el in part.iter_all(end=offset_divs): | ||
| if isinstance(el, score.Note): | ||
| if el.step == note_attributes["step"] and el.octave == note_attributes["octave"]: | ||
| el.tie_next = part_note | ||
| part_note.tie_prev = el | ||
|
|
There was a problem hiding this comment.
nice solution! some minor problems:
- the loop should break when a note is found
- the accidentals should be checked too for pitch matching
- the notes should only be tied if the end/start points match, maybe iter_ending is the way to go
- somehow handle cases without found note (drop note? add? warn?)
- the indentation level seems one too high
There was a problem hiding this comment.
Thanks for the feedback, points 1,2 are super important I will fix them ASAP
end=offset_divs should already cover the third point.
Warning would be the way to go for no note found.
There was a problem hiding this comment.
For your last point, currently, it only searches for ties when a note is added. The other clause already ties notes, so I am not sure that it should be there.
| feature_functions: Union[List, str], | ||
| add_idx: bool = False, | ||
| include_empty_features: bool = True, |
There was a problem hiding this comment.
the function now breaks if include_empty_features is False but no features can be found. Is this intended?
There was a problem hiding this comment.
Do you have a test file? So I can test while fixing it?
|
This PR addresses the following: New features
Bug fixes and related issues
TODO
Other changes
|
This PR addresses fixing uniformity in note features and Performance codec to accept ScoreLike objects and part merging for scores. It also adds the optional flag on note features to yield fixed size features.