Skip to content

Fix: Midi pcsetNearest not taking octave displacement into account#468

Merged
danigb merged 3 commits into
tonaljs:mainfrom
jeffgord:fix/pcset-nearest
Jun 4, 2025
Merged

Fix: Midi pcsetNearest not taking octave displacement into account#468
danigb merged 3 commits into
tonaljs:mainfrom
jeffgord:fix/pcset-nearest

Conversation

@jeffgord

Copy link
Copy Markdown
Contributor

I noticed a small bug with the pcsetNearest function in the Midi package. The issue is with the following code:

if (set.includes(ch + i)) return midi + i;
if (set.includes(ch - i)) return midi - i;

The problem here is that in certain situations ch + i could be greater than 11, or ch - i could be lower than 0. For an extreme example that demonstrates the issue, consider mapping the chromatic scale to a pcset of just a C and F# (i.e. 100000100000).

const nearest = Midi.pcsetNearest("100000100000");
[36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47].map(nearest);

With the old algorithm, this would give the following output:

[36, 36, 36, 42, 42, 42, 42, 42, 42, 42, 42, 42]

However, this is clearly wrong. The B natural (midi 47) is closer to a C than an F#, but is marked as being closest to the F#. This is because ch + i will be greater than 12 for any value of i > 0, but the pcset only includes chromas between 0 and 11. This causes subtler issues with more real-world examples. For example, consider mapping the chromatic scale to the nearest C minor pentatonic scale. The B natural will be marked as being nearest to the Bb (rather than C). While B natural is equidistant from Bb and C, the algorithm usually prefers the higher pitch in equidistant scenarios.

To fix this, I have modified the pcsetNearest algorithm to take into account octave displacement. I have also modified the tests, and added a few demonstrating the issue to take this into account. I think it might also make sense to add a boolean flag to the function preferHigherPitchWhenEquidistant, but I'll leave that for a separate PR.

@danigb danigb merged commit c167e66 into tonaljs:main Jun 4, 2025
0 of 2 checks passed
@danigb

danigb commented Jun 4, 2025

Copy link
Copy Markdown
Collaborator

Hey! Sorry, somehow this PR went below my radar. Thanks a lot!

@danigb

danigb commented Jun 4, 2025

Copy link
Copy Markdown
Collaborator

This is released in 6.4.2. Thanks again

@jeffgord

jeffgord commented Jun 6, 2025

Copy link
Copy Markdown
Contributor Author

Sweet - happy to contribute! I recognize you from the smplr library. You are doing awesome work for web-based music technology. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants