From 7e51e526bc82636d72891731bc3c14e8b056a4ea Mon Sep 17 00:00:00 2001 From: hill Date: Tue, 8 Sep 2026 19:59:02 +0200 Subject: [PATCH 1/2] Hand back a new pitch from getPitchFromNodeDegree realizeAscending() and realizeDescending() return the very list they put in _ascendingCache / _descendingCache, so the pitches inside it belong to the cache. getPitchFromNodeDegree() handed one of those pitches straight back, and nextPitch() writes the origin's octave onto the pitch it gets ("transfer octave from origin to new pitch derived from node"), so it was editing the cached realization in place. The effect is that a scale answers differently depending on what has been asked of it before: >>> sc = scale.RagAsawari('c4') >>> str(sc.pitchFromDegree(1)) 'C4' >>> str(sc.nextPitch('c1', scale.Direction.ASCENDING)) 'D1' >>> str(sc.pitchFromDegree(1)) 'C1' The cached entry for ('C4', 'C4', 'C5') has its first pitch moved from C4 to C1 by the walk near C1, and every later reader of that realization sees it. realize() already guards the list against this ("Make new objects, because this value might be cached ... and mutating it would be dangerous"); this is the same hazard one level down, on the pitches. test_scale_main.testRagAsawari asserted the corrupted 'C1'. It asserts 'C4' now, which is what the same call returns earlier in that very test. Two regression tests go with it: one that the pitch handed back is the caller's own, one for the reported symptom. The whole suite is unchanged otherwise: 4,597 tests, one failure fewer and nothing newly failing. --- music21/scale/intervalNetwork.py | 8 ++++++-- music21/scale/test_intervalNetwork.py | 27 +++++++++++++++++++++++++++ music21/scale/test_scale_main.py | 5 ++++- 3 files changed, 37 insertions(+), 3 deletions(-) diff --git a/music21/scale/intervalNetwork.py b/music21/scale/intervalNetwork.py index 4b7e5eaec..2f7678dd3 100644 --- a/music21/scale/intervalNetwork.py +++ b/music21/scale/intervalNetwork.py @@ -2707,15 +2707,19 @@ def getPitchFromNodeDegree( # environLocal.printDebug(['comparing', realizedNId, # 'nodeTargetId', nodeTargetId]) + # Return a new object: the realization this pitch came out of + # may be held in _ascendingCache or _descendingCache, and a + # caller that writes to the pitch -- nextPitch() sets its + # octave -- would edit the cached scale itself. if realizedNId == nodeTargetId.id: - return realizedPitch[i] + return copy.deepcopy(realizedPitch[i]) # NOTE: this condition may be too generous, and was added to solve # a non-tracked problem. # only match this generously if we are equating termini if equateTermini: if ((realizedNId in (Terminus.HIGH, Terminus.LOW)) and (nodeTargetId.id in (Terminus.HIGH, Terminus.LOW))): - return realizedPitch[i] + return copy.deepcopy(realizedPitch[i]) # environLocal.printDebug(['getPitchFromNodeDegree() on trial', trial, ', # failed to find node', nodeTargetId]) diff --git a/music21/scale/test_intervalNetwork.py b/music21/scale/test_intervalNetwork.py index da1f3061b..2c8ae0be5 100644 --- a/music21/scale/test_intervalNetwork.py +++ b/music21/scale/test_intervalNetwork.py @@ -627,6 +627,33 @@ def test_realize_descending_reversed_cached(self): self.assertEqual(descending_melodic_minor_reversed[0].nameWithOctave, 'C4') # was B-4 self.assertEqual(descending_melodic_minor_reversed[-1].nameWithOctave, 'B-4') # was C4 + def test_get_pitch_from_node_degree_returns_a_new_pitch(self): + ''' + A realization may be held in _ascendingCache or _descendingCache, so + the pitch taken out of one has to be the caller's own -- otherwise + writing to it edits the cached scale. + ''' + net = IntervalNetwork() + net.fillBiDirectedEdges(['M2', 'M2', 'm2', 'M2', 'M2', 'M2', 'm2']) + + first = net.getPitchFromNodeDegree('c4', 1, 1) + self.assertEqual(first.nameWithOctave, 'C4') + first.octave = 1 + + again = net.getPitchFromNodeDegree('c4', 1, 1) + self.assertEqual(again.nameWithOctave, 'C4') + + def test_next_pitch_does_not_move_a_cached_degree(self): + ''' + nextPitch() writes the origin's octave onto the pitch it gets back + from getPitchFromNodeDegree(), so walking the scale down near C1 used + to leave the tonic reading C1 ever after. + ''' + sc = scale.RagAsawari('c4') + self.assertEqual(str(sc.pitchFromDegree(1)), 'C4') + self.assertEqual(str(sc.nextPitch('c1', Direction.ASCENDING)), 'D1') + self.assertEqual(str(sc.pitchFromDegree(1)), 'C4') + # ------------------------------------------------------------------------------ if __name__ == '__main__': diff --git a/music21/scale/test_scale_main.py b/music21/scale/test_scale_main.py index bc3826c0f..e57e4c245 100644 --- a/music21/scale/test_scale_main.py +++ b/music21/scale/test_scale_main.py @@ -456,7 +456,10 @@ def testRagAsawari(self): getNeighbor=Direction.DESCENDING)), 'F1') - self.assertEqual(str(sc.pitchFromDegree(1)), 'C1') + # the tonic, whatever has been asked for in between: walking the + # scale near another octave used to move it, since nextPitch() was + # writing an octave onto a pitch of the cached realization. + self.assertEqual(str(sc.pitchFromDegree(1)), 'C4') # there is no third step in ascending form self.assertEqual(str(sc.pitchFromDegree(3)), 'None') self.assertEqual(str(sc.pitchFromDegree(3, direction=Direction.DESCENDING)), From 8e51e9c3ff047e9377baf7deef5348ed1518b900 Mon Sep 17 00:00:00 2001 From: hill Date: Thu, 10 Sep 2026 15:28:05 +0200 Subject: [PATCH 2/2] Leave the copying to the network getPitchFromNodeDegree hands back a new pitch, so the AbstractScale wrappers over it, getPitchFromNodeDegree and getNewTonicPitch, no longer need a copy of their own. The comments in intervalNetwork, test_intervalNetwork and testRagAsawari told the story of the bug that copy fixed; the fix's own commit already tells it, so they go. --- music21/scale/__init__.py | 4 ++-- music21/scale/intervalNetwork.py | 5 +---- music21/scale/test_intervalNetwork.py | 5 ----- music21/scale/test_scale_main.py | 3 --- 4 files changed, 3 insertions(+), 14 deletions(-) diff --git a/music21/scale/__init__.py b/music21/scale/__init__.py index b48563c63..ebb66478e 100644 --- a/music21/scale/__init__.py +++ b/music21/scale/__init__.py @@ -557,7 +557,7 @@ def getPitchFromNodeDegree(self, alteredDegrees=self._alteredDegrees, equateTermini=equateTermini ) - return copy.deepcopy(post) + return post def realizePitchByDegree(self, pitchReference: _PitchOrStr, @@ -654,7 +654,7 @@ def getNewTonicPitch(self, maxPitch=maxPitch, alteredDegrees=self._alteredDegrees ) - return copy.deepcopy(post) + return post # -------------------------------------------------------------------------- diff --git a/music21/scale/intervalNetwork.py b/music21/scale/intervalNetwork.py index 2f7678dd3..d7e97b2ad 100644 --- a/music21/scale/intervalNetwork.py +++ b/music21/scale/intervalNetwork.py @@ -2707,10 +2707,7 @@ def getPitchFromNodeDegree( # environLocal.printDebug(['comparing', realizedNId, # 'nodeTargetId', nodeTargetId]) - # Return a new object: the realization this pitch came out of - # may be held in _ascendingCache or _descendingCache, and a - # caller that writes to the pitch -- nextPitch() sets its - # octave -- would edit the cached scale itself. + # realizedPitch may be a cached realization: hand back a copy if realizedNId == nodeTargetId.id: return copy.deepcopy(realizedPitch[i]) # NOTE: this condition may be too generous, and was added to solve diff --git a/music21/scale/test_intervalNetwork.py b/music21/scale/test_intervalNetwork.py index 2c8ae0be5..6e2743521 100644 --- a/music21/scale/test_intervalNetwork.py +++ b/music21/scale/test_intervalNetwork.py @@ -644,11 +644,6 @@ def test_get_pitch_from_node_degree_returns_a_new_pitch(self): self.assertEqual(again.nameWithOctave, 'C4') def test_next_pitch_does_not_move_a_cached_degree(self): - ''' - nextPitch() writes the origin's octave onto the pitch it gets back - from getPitchFromNodeDegree(), so walking the scale down near C1 used - to leave the tonic reading C1 ever after. - ''' sc = scale.RagAsawari('c4') self.assertEqual(str(sc.pitchFromDegree(1)), 'C4') self.assertEqual(str(sc.nextPitch('c1', Direction.ASCENDING)), 'D1') diff --git a/music21/scale/test_scale_main.py b/music21/scale/test_scale_main.py index e57e4c245..ded1bd6d9 100644 --- a/music21/scale/test_scale_main.py +++ b/music21/scale/test_scale_main.py @@ -456,9 +456,6 @@ def testRagAsawari(self): getNeighbor=Direction.DESCENDING)), 'F1') - # the tonic, whatever has been asked for in between: walking the - # scale near another octave used to move it, since nextPitch() was - # writing an octave onto a pitch of the cached realization. self.assertEqual(str(sc.pitchFromDegree(1)), 'C4') # there is no third step in ascending form self.assertEqual(str(sc.pitchFromDegree(3)), 'None')