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

Make _dissonanceScore independent of octaves #1695

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

TimFelixBeyer
Copy link
Contributor

@TimFelixBeyer TimFelixBeyer commented Mar 3, 2024

Fixes #1689 where the spelling of a pitch was dependent on its octave.

This was a result of the _dissonanceScore computing Pythagorean ratios between the keyContext (which contributes a Pitch without Octave), and the given pitches. Now, the ratios are always computed between Pitches without Octaves to ensure correct scoring.

Adds a regression test and code simplifications as well.

Fixes cuthbertLab#1689 where the spelling of a pitch was dependent on its octave.
@coveralls
Copy link

coveralls commented Mar 3, 2024

Coverage Status

coverage: 93.032%. remained the same
when pulling 6371669 on TimFelixBeyer:dissonance-score-fix
into 809ba9d on cuthbertLab:master.

float3 added a commit to float3/tuningplayground that referenced this pull request Mar 13, 2024
@mscuthbert
Copy link
Member

I'm not sure I agree with this approach. Dissonances are generally more tolerated in higher pitches than lower pitches. If we're going to need to place a dissonance somewhere I'd generally place it higher. Is there a test that shows an improvement with this method?

@mscuthbert
Copy link
Member

closing and reopening to update lint.

@mscuthbert mscuthbert closed this Apr 25, 2024
@mscuthbert mscuthbert reopened this Apr 25, 2024
@TimFelixBeyer
Copy link
Contributor Author

The linked issue (#1689) was the main motivation for this PR, and provides an example that is fixed.
I see that I forgot to include the case from there as a test but would be happy to add that.
If #1689 is intended behavior, then this PR should not be merged.
LMK how you'd like to proceed!

@mscuthbert
Copy link
Member

Ah, thanks -- sorry that I missed this. Yes, that definitely is a problem -- no way that Gb is the right spelling for a single note in Am. Thanks for the fix.

@mscuthbert mscuthbert merged commit 3fae7a1 into cuthbertLab:master Apr 29, 2024
15 of 19 checks passed
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.

simplifyMultipleEnharmonics gives different spellings in different octaves
3 participants