From c3045287d901768fd1418c78559bf84c0dddb105 Mon Sep 17 00:00:00 2001 From: hill Date: Tue, 8 Sep 2026 02:24:39 +0200 Subject: [PATCH 1/4] Tell a negative octave from a flat in removeRedundantPitches `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. --- music21/chord/__init__.py | 59 ++++++++++++++++++++------------------ music21/test/test_chord.py | 28 ++++++++++++++++++ 2 files changed, 59 insertions(+), 28 deletions(-) diff --git a/music21/chord/__init__.py b/music21/chord/__init__.py index a0d6789f6..1539deb35 100644 --- a/music21/chord/__init__.py +++ b/music21/chord/__init__.py @@ -964,17 +964,26 @@ def _removePitchByRedundantAttribute( ''' Common method for stripping pitches based on redundancy of one pitch attribute. The `attribute` is provided by a string. + + `'nameWithOctave'` is a special case: the string it gives spells + B-flat in octave 1 and B-natural in octave -1 alike, so two pitches + are compared by their name and octave instead of by it. ''' if not inPlace: # make a copy returnObj = copy.deepcopy(self) else: returnObj = self - uniquePitches = [] + uniquePitches: list[t.Any] = [] deleteComponents = [] for comp in returnObj._notes: - if getattr(comp.pitch, attribute) not in uniquePitches: - uniquePitches.append(getattr(comp.pitch, attribute)) + if attribute == 'nameWithOctave': + value = (comp.pitch.name, comp.pitch.octave, comp.pitch.octaveIsImplicit) + else: + value = getattr(comp.pitch, attribute) + + if value not in uniquePitches: + uniquePitches.append(value) else: deleteComponents.append(comp) @@ -3615,41 +3624,35 @@ def removeRedundantPitches(self, *, inPlace: bool = False) -> t.Self|list[pitch. >>> c2c - It is a known bug that because pitch.nameWithOctave gives - the same value for B-flat in octave 1 as B-natural in octave - negative 1, negative octaves can screw up this method. - With all the things left to do for music21, it doesn't seem - a bug worth squashing at this moment, but FYI: + Two pitches count as the same when they have the same name in the same + octave. Note that this is a finer distinction than + :attr:`~music21.pitch.Pitch.nameWithOctave`, whose '-' is the flat sign + and the minus sign both: B-flat in octave 1 and B-natural in octave -1 + are two different pitches that spell alike, and both are kept. >>> p1 = pitch.Pitch('B-') >>> p1.octave = 1 >>> p2 = pitch.Pitch('B') >>> p2.octave = -1 - >>> c3 = chord.Chord([p1, p2]) - >>> removedPitches = c3.removeRedundantPitches(inPlace=True) - >>> c3.pitches - (,) - - >>> c3.pitches[0].name - 'B-' - >>> c3.pitches[0].octave - 1 - >>> removedPitches - [] - >>> removedPitches[0].name - 'B' - >>> removedPitches[0].octave - -1 + >>> p1.nameWithOctave == p2.nameWithOctave + True - The first pitch survives: + >>> c3 = chord.Chord([p1, p2]) + >>> c3.removeRedundantPitches(inPlace=True) + [] + >>> [(p.name, p.octave) for p in c3.pitches] + [('B-', 1), ('B', -1)] - >>> c3.pitches[0] is p1 - True + A pitch with no octave of its own is likewise not the same pitch as + one placed in the default octave: - >>> c3.pitches[0] is p2 - False + >>> c4 = chord.Chord([pitch.Pitch('C'), pitch.Pitch('C4')]) + >>> c4.removeRedundantPitches() + * Changed in v6: inPlace defaults to False. + * Changed in v11: a negative octave is no longer read as a flat, so + B-natural in octave -1 survives beside B-flat in octave 1. ''' return self._removePitchByRedundantAttribute('nameWithOctave', inPlace=inPlace) diff --git a/music21/test/test_chord.py b/music21/test/test_chord.py index caa50790e..25f0e94d0 100644 --- a/music21/test/test_chord.py +++ b/music21/test/test_chord.py @@ -739,6 +739,34 @@ def testChordCannotContainUnpitched(self): # noinspection PyTypeChecker Chord([note.Unpitched()]) # type: ignore + def testRemoveRedundantPitchesTellsNegativeOctavesFromFlats(self): + ''' + nameWithOctave spells both B-flat in octave 1 and B-natural in + octave -1 as 'B-1', so comparing pitches by that string made the + second look like a repeat of the first. + ''' + flat = pitch.Pitch('B-') + flat.octave = 1 + natural = pitch.Pitch('B') + natural.octave = -1 + self.assertEqual(flat.nameWithOctave, natural.nameWithOctave) + + ch = Chord([flat, natural]) + self.assertEqual(ch.removeRedundantPitches(inPlace=True), []) + self.assertEqual([(p.name, p.octave) for p in ch.pitches], + [('B-', 1), ('B', -1)]) + + # a real repeat still goes, whatever octave it is in + repeated = Chord([flat, natural, pitch.Pitch('B-1')]) + removed = repeated.removeRedundantPitches(inPlace=True) + self.assertEqual([p.nameWithOctave for p in removed], ['B-1']) + self.assertEqual(len(repeated.pitches), 2) + + # and a pitch with no octave is not the one in the default octave + octaveless = Chord([pitch.Pitch('C'), pitch.Pitch('C4')]) + self.assertEqual(octaveless.removeRedundantPitches(inPlace=True), []) + self.assertEqual(len(octaveless.pitches), 2) + def testCacheClearedOnAdd(self): ch = chord.Chord('C4 E4 G4') self.assertTrue(ch.isConsonant()) From a82bde2f57e19b4bfa04eda9a64a27aa2171e580 Mon Sep 17 00:00:00 2001 From: Michael Scott Asato Cuthbert Date: Mon, 7 Sep 2026 15:43:42 -1000 Subject: [PATCH 2/4] Clearer Docs Remove docs that overemphasize this bug fix compared to the routine's work as a whole. --- music21/chord/__init__.py | 40 +++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/music21/chord/__init__.py b/music21/chord/__init__.py index 1539deb35..a5de5b801 100644 --- a/music21/chord/__init__.py +++ b/music21/chord/__init__.py @@ -965,25 +965,25 @@ def _removePitchByRedundantAttribute( Common method for stripping pitches based on redundancy of one pitch attribute. The `attribute` is provided by a string. - `'nameWithOctave'` is a special case: the string it gives spells - B-flat in octave 1 and B-natural in octave -1 alike, so two pitches - are compared by their name and octave instead of by it. + attribute `'nameWithOctave'` uses a special case to make comparisons with + negative octaves work properly ''' if not inPlace: # make a copy returnObj = copy.deepcopy(self) else: returnObj = self - uniquePitches: list[t.Any] = [] + uniquePitches: set[t.Any] = set() deleteComponents = [] for comp in returnObj._notes: if attribute == 'nameWithOctave': + # for comparing with negative octaves etc. value = (comp.pitch.name, comp.pitch.octave, comp.pitch.octaveIsImplicit) else: value = getattr(comp.pitch, attribute) if value not in uniquePitches: - uniquePitches.append(value) + uniquePitches.add(value) else: deleteComponents.append(comp) @@ -3624,24 +3624,25 @@ def removeRedundantPitches(self, *, inPlace: bool = False) -> t.Self|list[pitch. >>> c2c - Two pitches count as the same when they have the same name in the same - octave. Note that this is a finer distinction than - :attr:`~music21.pitch.Pitch.nameWithOctave`, whose '-' is the flat sign - and the minus sign both: B-flat in octave 1 and B-natural in octave -1 - are two different pitches that spell alike, and both are kept. - - >>> p1 = pitch.Pitch('B-') - >>> p1.octave = 1 + Pitches with extreme octaves (whose `.nameWithOctave`s + look identical) are still distinguished. + + >>> p1 = pitch.Pitch('B') + >>> p1.octave = -1 >>> p2 = pitch.Pitch('B') - >>> p2.octave = -1 - >>> p1.nameWithOctave == p2.nameWithOctave - True + >>> p2.octave = 1 >>> c3 = chord.Chord([p1, p2]) - >>> c3.removeRedundantPitches(inPlace=True) + >>> c3 + + + >>> removedPitches = c3.removeRedundantPitches(inPlace=True) + >>> removedPitches [] + >>> c3 + >>> [(p.name, p.octave) for p in c3.pitches] - [('B-', 1), ('B', -1)] + [('B', -1), ('B', 1)] A pitch with no octave of its own is likewise not the same pitch as one placed in the default octave: @@ -3651,8 +3652,7 @@ def removeRedundantPitches(self, *, inPlace: bool = False) -> t.Self|list[pitch. * Changed in v6: inPlace defaults to False. - * Changed in v11: a negative octave is no longer read as a flat, so - B-natural in octave -1 survives beside B-flat in octave 1. + * Changed in v11: works properly with negative octaves ''' return self._removePitchByRedundantAttribute('nameWithOctave', inPlace=inPlace) From 5488dc2b079d2becee7e7d190f51d102e9a6ad5c Mon Sep 17 00:00:00 2001 From: hill Date: Tue, 8 Sep 2026 09:06:01 +0200 Subject: [PATCH 3/4] Make the docstring example collide again, and green The example had been changed to B-natural in octave -1 against B-natural in octave 1. Those spell 'B-1' and 'B1', which do not look alike, so the example no longer showed what it says it shows, and the two `>>> c3` lines expected `` where the chord actually reprs as ``. Back to B-flat in octave 1 against B-natural in octave -1, which is the pair that collides: both spell 'B-1', the chord really does repr as two of them, and the wording about `.nameWithOctave`s looking identical is true again. Kept the shape of the example as it was rewritten. Also drops the whitespace from two blank lines, which ruff was failing on. --- music21/chord/__init__.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/music21/chord/__init__.py b/music21/chord/__init__.py index a5de5b801..f15c07c1f 100644 --- a/music21/chord/__init__.py +++ b/music21/chord/__init__.py @@ -3625,24 +3625,28 @@ def removeRedundantPitches(self, *, inPlace: bool = False) -> t.Self|list[pitch. Pitches with extreme octaves (whose `.nameWithOctave`s - look identical) are still distinguished. - - >>> p1 = pitch.Pitch('B') - >>> p1.octave = -1 + look identical) are still distinguished. B-flat in octave 1 and + B-natural in octave -1 both spell 'B-1', the '-' being the flat sign + in the one and the minus sign in the other: + + >>> p1 = pitch.Pitch('B-') + >>> p1.octave = 1 >>> p2 = pitch.Pitch('B') - >>> p2.octave = 1 + >>> p2.octave = -1 + >>> p1.nameWithOctave == p2.nameWithOctave + True >>> c3 = chord.Chord([p1, p2]) >>> c3 - + >>> removedPitches = c3.removeRedundantPitches(inPlace=True) >>> removedPitches [] >>> c3 >>> [(p.name, p.octave) for p in c3.pitches] - [('B', -1), ('B', 1)] + [('B-', 1), ('B', -1)] A pitch with no octave of its own is likewise not the same pitch as one placed in the default octave: From 877b65f832ee44a8fdf753276d922badb307d0be Mon Sep 17 00:00:00 2001 From: Michael Scott Asato Cuthbert Date: Tue, 8 Sep 2026 10:43:16 -1000 Subject: [PATCH 4/4] restore my phrasing Restore phrasing --- music21/chord/__init__.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/music21/chord/__init__.py b/music21/chord/__init__.py index f15c07c1f..83977eae3 100644 --- a/music21/chord/__init__.py +++ b/music21/chord/__init__.py @@ -3625,14 +3625,12 @@ def removeRedundantPitches(self, *, inPlace: bool = False) -> t.Self|list[pitch. Pitches with extreme octaves (whose `.nameWithOctave`s - look identical) are still distinguished. B-flat in octave 1 and - B-natural in octave -1 both spell 'B-1', the '-' being the flat sign - in the one and the minus sign in the other: - - >>> p1 = pitch.Pitch('B-') - >>> p1.octave = 1 - >>> p2 = pitch.Pitch('B') - >>> p2.octave = -1 + look identical) are still distinguished. + + >>> p1 = pitch.Pitch('B') + >>> p1.octave = -1 + >>> p2 = pitch.Pitch('B-') + >>> p2.octave = 1 >>> p1.nameWithOctave == p2.nameWithOctave True @@ -3646,7 +3644,7 @@ def removeRedundantPitches(self, *, inPlace: bool = False) -> t.Self|list[pitch. >>> c3 >>> [(p.name, p.octave) for p in c3.pitches] - [('B-', 1), ('B', -1)] + [('B', -1), ('B-', 1)] A pitch with no octave of its own is likewise not the same pitch as one placed in the default octave: