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

Fix issue #166 (synthesizing scores with pickup measures) #167

Merged
merged 12 commits into from
Oct 25, 2022

Conversation

neosatrapahereje
Copy link
Member

This pull request contains the following changes

  • It solves issue Bug synthesizing scores with pickup measures #166 (synthesizing scores with pickup measures)
  • It allows for using parameters for the tuning functions directly in the synthesize function
  • It adds a new performance_note_array_from_score_note_array which allows for more flexibility converting score-like note arrays to performance-like note arrays (including adding extra features for non-constant tempo and velocity in performance_from_part)

@neosatrapahereje neosatrapahereje changed the title Fix bug synthesizing Fix issue #166 (synthesizing scores with pickup measures) Oct 22, 2022
@neosatrapahereje neosatrapahereje self-assigned this Oct 22, 2022
@neosatrapahereje neosatrapahereje added the bug Something isn't working label Oct 22, 2022
@neosatrapahereje neosatrapahereje linked an issue Oct 22, 2022 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented Oct 22, 2022

Codecov Report

Base: 80.53% // Head: 81.17% // Increases project coverage by +0.63% 🎉

Coverage data is based on head (2e3fe88) compared to base (08279af).
Patch coverage: 93.84% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #167      +/-   ##
===========================================
+ Coverage    80.53%   81.17%   +0.63%     
===========================================
  Files           63       63              
  Lines        10313    10488     +175     
===========================================
+ Hits          8306     8514     +208     
+ Misses        2007     1974      -33     
Impacted Files Coverage Δ
partitura/utils/synth.py 72.79% <60.00%> (-0.49%) ⬇️
tests/test_utils.py 96.92% <96.10%> (-3.08%) ⬇️
partitura/utils/music.py 72.20% <96.87%> (+3.97%) ⬆️
partitura/io/exportaudio.py 92.85% <100.00%> (+0.54%) ⬆️
tests/test_synth.py 97.77% <100.00%> (-1.01%) ⬇️
tests/__init__.py 100.00% <0.00%> (ø)
partitura/score.py 79.55% <0.00%> (ø)
tests/test_kern.py 100.00% <0.00%> (ø)
... and 52 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.


Potential extensions
--------------------
* allow for bpm to be a callable or an 2D array with columns (onset, bpm)
Copy link
Member Author

Choose a reason for hiding this comment

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

This pull request implements these extensions (now as part of the performance_notearray_from_score_notearray method below)

@neosatrapahereje neosatrapahereje added this to In progress in Partitura 1.1.1 via automation Oct 22, 2022
@manoskary manoskary mentioned this pull request Oct 24, 2022
@sildater sildater self-requested a review October 25, 2022 08:07
semitones, while "natural" will use natural ratios within an octave.
Otherwise it uses a callable. See `midi_pitch_to_natural_frequency`
and `midi_pitch_to_frequency` for more info on the "equal_temperament"
and "natural" tuning.
Copy link
Member

Choose a reason for hiding this comment

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

natural temperament is based on the reference pitch of A, this choice is not clearly documented and gives wrong results for many pieces where the base note should be different. I see two ways of fixing this, 1) using the midi_pitch_to_tempered_frequency function instead or 2) clearly documenting that this renders a version based on reference pitch A with interval ratios given in NATURAL_INTERVAL_RATIOS

Copy link
Member

Choose a reason for hiding this comment

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

I added a symmetric 5-limit temperament table FIVE_LIMIT_INTERVAL_RATIOS

Copy link
Member Author

Choose a reason for hiding this comment

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

The new changes reflect both suggestions:

  1. The function midi_pitch_to_tempered_frequency is now used instead of midi_pitch_to_natural_frequency if tuning="natural"
  2. The docstring explicitly states that the function computes intervals with respect to a reference pitch and that the FIVE_LIMIT_INTERVAL_RATIOS are used by default.

@sildater sildater self-requested a review October 25, 2022 19:09
Partitura 1.1.1 automation moved this from In progress to Reviewer approved Oct 25, 2022
@sildater sildater merged commit 9e3992d into develop Oct 25, 2022
Partitura 1.1.1 automation moved this from Reviewer approved to Done Oct 25, 2022
@sildater sildater deleted the parametrize_tuning_synth branch October 25, 2022 19:17
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.

Bug synthesizing scores with pickup measures
3 participants