Merge pull request #1311 from cuthbertLab/cache-scale

Fix `getPitches(direction=DESCENDING)` returning ascending scales
cuthbertLab · May 23, 2022 · 562aede · 562aede
2 parents c39094a + 110802e
commit 562aede
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 9 deletions.
diff --git a/music21/scale/intervalNetwork.py b/music21/scale/intervalNetwork.py
@@ -72,7 +72,8 @@ def __str__(self):
         return 'Direction.' + self.name
 
 
-CacheKey = t.Tuple[t.Union[int, Terminus], str, t.Union[str, None], t.Union[str, None], bool]
+CacheKey = t.Tuple[
+    t.Union[int, Terminus], str, t.Union[str, None], t.Union[str, None], bool, t.Optional[bool]]
 
 
 def _gte(a, b):
@@ -1341,6 +1342,7 @@ def _getCacheKey(
         minPitch: t.Optional[pitch.Pitch],
         maxPitch: t.Optional[pitch.Pitch],
         includeFirst: bool,
+        reverse: t.Optional[bool] = None,  # only meaningful for descending
     ) -> CacheKey:
         '''
         Return key for caching based on critical components.
@@ -1358,7 +1360,9 @@ def _getCacheKey(
                 pitchReference.nameWithOctave,
                 minKey,
                 maxKey,
-                includeFirst)
+                includeFirst,
+                reverse,
+                )
 
     def realizeAscending(
         self,
@@ -1635,7 +1639,9 @@ def realizeDescending(
                                    pitchRef,
                                    minPitchObj,
                                    maxPitchObj,
-                                   includeFirst)
+                                   includeFirst,
+                                   reverse,
+                                   )
             if ck in self._descendingCache:
                 return self._descendingCache[ck]
 
@@ -1913,8 +1919,10 @@ def realize(self,
             mergedPitches, mergedNodes = pre + post, preNodeId + postNodeId
 
         if reverse:
-            mergedPitches.reverse()
-            mergedNodes.reverse()
+            # Make new objects, because this value might be cached in intervalNetwork's
+            # _descendingCache, and mutating it would be dangerous.
+            mergedPitches = list(reversed(mergedPitches))
+            mergedNodes = list(reversed(mergedNodes))
 
         return mergedPitches, mergedNodes
 

diff --git a/music21/scale/test_intervalNetwork.py b/music21/scale/test_intervalNetwork.py
@@ -594,6 +594,20 @@ def testNextPitch(self):
                 getNeighbor=Direction.DESCENDING)),
             'A-4')
 
+    def test_realize_descending_reversed_cached(self):
+        net = IntervalNetwork()
+        net.fillMelodicMinor()
+
+        descending_melodic_minor, _ = net.realizeDescending(
+            'C4', minPitch='C4', maxPitch='C5', reverse=False)
+        self.assertEqual(descending_melodic_minor[0].nameWithOctave, 'B-4')
+        self.assertEqual(descending_melodic_minor[-1].nameWithOctave, 'C4')
+
+        descending_melodic_minor_reversed, _ = net.realizeDescending(
+            'C4', minPitch='C4', maxPitch='C5', reverse=True)
+        self.assertEqual(descending_melodic_minor_reversed[0].nameWithOctave, 'C4')  # was B-4
+        self.assertEqual(descending_melodic_minor_reversed[-1].nameWithOctave, 'B-4')  # was C4
+
 
 # ------------------------------------------------------------------------------
 if __name__ == '__main__':

diff --git a/music21/scale/test_scale_main.py b/music21/scale/test_scale_main.py
@@ -302,16 +302,13 @@ def testMelodicMinorA(self):
         self.assertEqual(self.pitchOut(mm.getPitches('c1', 'c3', direction=Direction.DESCENDING)),
                          '[C3, B2, A2, G2, F2, E2, D2, C2, B1, A1, G1, F1, E1, D1, C1]')
 
-        # TODO: this shows a problem with a bidirectional scale: we are
-        # always starting at the tonic and moving up or down; so this is still
-        # giving a descended portion, even though an ascending portion was requested
         # noinspection PyArgumentList
         self.assertEqual(self.pitchOut(mm.getPitches('c1', 'c3', direction=Direction.ASCENDING)),
                          '[C1, D1, E1, F#1, G#1, A1, B1, C2, D2, E2, F#2, G#2, A2, B2, C3]')
 
         # noinspection PyArgumentList
         self.assertEqual(self.pitchOut(mm.getPitches('c1', 'c3', direction=Direction.DESCENDING)),
-                         '[C1, D1, E1, F1, G1, A1, B1, C2, D2, E2, F2, G2, A2, B2, C3]')
+                         '[C3, B2, A2, G2, F2, E2, D2, C2, B1, A1, G1, F1, E1, D1, C1]')
 
         # noinspection PyArgumentList
         self.assertEqual(self.pitchOut(mm.getPitches('a5', 'a6', direction=Direction.ASCENDING)),
@@ -903,6 +900,19 @@ def testDerivedScaleAbsurdOctaves(self):
         with self.assertRaises(intervalNetwork.IntervalNetworkException):
             e.deriveRanked(['C4', 'E4', 'G4'], comparisonAttribute='name')
 
+    def test_getPitches_multiple_times(self):
+        """Worth testing because we found cached lists being mutated."""
+        c_maj = scale.MajorScale('C')
+
+        for i in range(3):  # catch both even and odd states
+            with self.subTest(iterations=i):
+                # due to a tight coupling of direction and reverse, this sets reverse=True
+                # when calling getRealization()
+                post = c_maj.getPitches(direction=Direction.DESCENDING)
+
+                self.assertEqual(post[0].nameWithOctave, 'C5')
+                self.assertEqual(post[-1].nameWithOctave, 'C4')
+
 
 # ------------------------------------------------------------------------------
 if __name__ == '__main__':