Tell a negative octave from a flat in removeRedundantPitches - #2027
Open
float3 wants to merge 2 commits into
Open
Tell a negative octave from a flat in removeRedundantPitches#2027float3 wants to merge 2 commits into
float3 wants to merge 2 commits into
Conversation
Member
|
I would rather have removeByRedunantPitches have special knowledge of 'nameWithOctave' and substitute 'nameWithOctave' with a special routine (either as a tuple or not) -- make the equivalence not something users have to think about. Based on that, do not demonstrate ('name', 'octave') as the usage of a tuple in the docs -- I was trying to figure out some other usage of the tuple form -- but i can't think of one -- everything i tried ('pitchClass', 'octave') had an equivalent single attribute like |
`Pitch.nameWithOctave` spells both B-flat in octave 1 and B-natural in
octave -1 as 'B-1', because the '-' is the flat sign and the minus sign
both. `removeRedundantPitches` compared pitches by that string, so the
second read as a repeat of the first and was thrown away.
`_removePitchByRedundantAttribute` now knows that 'nameWithOctave' is the
ambiguous one and compares by name and octave in its place. Its signature
is unchanged and nothing new is exposed: the pitch-class and pitch-name
reductions, and both call sites, are exactly as they were.
The octave is compared together with `octaveIsImplicit`, so a pitch given
no octave stays distinct from one placed in the default octave, which is
what `nameWithOctave` ('C' against 'C4') already did.
The docstring documented the old behaviour as a known bug ("doesn't seem a
bug worth squashing at this moment"); it now documents what the method
does, and test_chord carries a regression test.
float3
force-pushed
the
fix-redundant-pitches-negative-octave
branch
from
September 8, 2026 00:24
6b1d83f to
c304528
Compare
Remove docs that overemphasize this bug fix compared to the routine's work as a whole.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pitch.nameWithOctavespells both B-flat in octave 1 and B-natural in octave -1 as'B-1', because the'-'is the flat sign and the minus sign both.removeRedundantPitchescompared pitches by that string, so the second read as a repeat of the first and was thrown away.Revised after review:
_removePitchByRedundantAttributenow has the special knowledge of'nameWithOctave'itself and compares by name and octave in its place. Its signature is unchanged, no tuple form is exposed, and both call sites and the pitch-class and pitch-name reductions are exactly as they were.While rewriting it I noticed the earlier version had a second problem, so this one also pins it: the octave is compared together with
octaveIsImplicit, so a pitch given no octave stays distinct from one placed in the default octave.nameWithOctavealready made that distinction ('C'against'C4'), and since v11Pitch.octavereturns 4 for both, comparing the octave alone would have quietly merged them. There is a doctest and a test case for it.The docstring documented the old behaviour as a known bug ("doesn't seem a bug worth squashing at this moment"); it now documents what the method does, and
test_chordcarries a regression test.