From fcb9fcf99d47b305b67eab3847b0c3dfdb716072 Mon Sep 17 00:00:00 2001 From: Michael Scott Asato Cuthbert Date: Mon, 1 Jun 2026 19:49:22 -1000 Subject: [PATCH 1/2] Fix roman.RomanNumeral Key cache colliding across modes The _keyCache in roman.py was keyed by Key.tonicPitchNameWithCase, which only encodes major (upper case) vs minor (lower case). Any other mode (lydian, dorian, etc.) falls back to the upper-case tonic, so a C lydian Key was cached under 'C' and collided with C major: after creating a RomanNumeral in C lydian, a later RomanNumeral('I', 'C') returned the cached lydian Key instead of C major. Add a _keyCacheKey() helper used at both cache-store sites: major/minor keep the case-only form so fast string lookups still hit, and any other mode is qualified with its mode name (e.g. 'C lydian') so it cannot be returned for a C major request. Add deterministic regression tests that clear the cache and check both orderings and the resulting cache keys. AI-assisted (Claude) --- music21/roman.py | 80 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 77 insertions(+), 3 deletions(-) diff --git a/music21/roman.py b/music21/roman.py index 53af1afa5..5626f0970 100644 --- a/music21/roman.py +++ b/music21/roman.py @@ -70,6 +70,22 @@ ) +def _keyCacheKey(keyObj: key.Key) -> str: + ''' + Return the string used to cache a Key in `_keyCache`. + + `tonicPitchNameWithCase` only distinguishes major (upper case) from minor + (lower case); every other mode falls back to the upper-case tonic, which + collides with the major key on the same tonic. For major and minor we keep + the case-only form so that string lookups like ``_getKeyFromCache('C')`` + still hit; any other mode is qualified with its mode name so that, for + example, ``C lydian`` cannot be returned when ``C major`` was requested. + ''' + if keyObj.mode in ('major', 'minor'): + return keyObj.tonicPitchNameWithCase + return str(keyObj) + + def _getKeyFromCache(keyStr: str) -> key.Key: ''' get a key from the cache if it is there; otherwise @@ -83,7 +99,7 @@ def _getKeyFromCache(keyStr: str) -> key.Key: keyObj = copy.copy(_keyCache[keyStr]) else: keyObj = key.Key(keyStr) - _keyCache[keyObj.tonicPitchNameWithCase] = keyObj + _keyCache[_keyCacheKey(keyObj)] = keyObj return keyObj @@ -3470,9 +3486,10 @@ def key(self, keyOrScale): ) if 'Key' in keyClasses: # good to go - if keyOrScale.tonicPitchNameWithCase not in _keyCache: + cacheKey = _keyCacheKey(keyOrScale) + if cacheKey not in _keyCache: # store for later - _keyCache[keyOrScale.tonicPitchNameWithCase] = keyOrScale + _keyCache[cacheKey] = keyOrScale elif 'Scale' in keyClasses: # Need unique 'hash' for each scale. Names is not enough as there are # multiple scales with the same name but different pitches. @@ -4555,6 +4572,63 @@ def test_scale_caching(self): rn = mcs.romanNumeral('IV7') self.assertEqual([p.unicodeName for p in rn.pitches], ['F', 'A', 'C', 'E♭']) + def test_key_cache_mode_not_just_name(self): + ''' + Regression test: the Key cache must distinguish modes that share a + tonic name, not just upper/lower case. Previously a `C lydian` Key + was cached under 'C' (because `tonicPitchNameWithCase` falls back to + the upper-case tonic for non-major/minor modes), so a subsequent + request for `C major` would return the cached lydian Key. + + This test was AI-assisted. + ''' + # lydian requested first, then major + _keyCache.clear() + _scaleCache.clear() + lydianFirst = RomanNumeral('I', key.Key('C', 'lydian')) + self.assertEqual(lydianFirst.key.mode, 'lydian') + majorAfter = RomanNumeral('I', 'C') + self.assertEqual(majorAfter.key.mode, 'major') + self.assertEqual([p.name for p in majorAfter.pitches], ['C', 'E', 'G']) + + # major requested first, then lydian (the reverse order) + _keyCache.clear() + _scaleCache.clear() + majorFirst = RomanNumeral('I', 'C') + self.assertEqual(majorFirst.key.mode, 'major') + lydianAfter = RomanNumeral('I', key.Key('C', 'lydian')) + self.assertEqual(lydianAfter.key.mode, 'lydian') + + # both modes can be cached simultaneously without clobbering each other + _keyCache.clear() + _scaleCache.clear() + cLydian = RomanNumeral('I', key.Key('C', 'lydian')) + cMajor = RomanNumeral('I', key.Key('C', 'major')) + cMinor = RomanNumeral('i', key.Key('c', 'minor')) + self.assertEqual(cLydian.key.mode, 'lydian') + self.assertEqual(cMajor.key.mode, 'major') + self.assertEqual(cMinor.key.mode, 'minor') + + # all three live under distinct cache keys; the mode-qualified 'C lydian' + # entry does not clobber the case-only 'C' (major) / 'c' (minor) entries + self.assertEqual(set(_keyCache), {'C', 'c', 'C lydian'}) + self.assertEqual(_keyCache['C'].mode, 'major') + self.assertEqual(_keyCache['c'].mode, 'minor') + self.assertEqual(_keyCache['C lydian'].mode, 'lydian') + + def test_get_key_from_cache_mode(self): + ''' + `_getKeyFromCache` builds major/minor keys from string case and must + not be polluted by exotic-mode Keys stored under the bare tonic name. + + This test was AI-assisted. + ''' + _keyCache.clear() + # populate the cache with a non-major/minor mode keyed on tonic 'C' + _keyCache[_keyCacheKey(key.Key('C', 'lydian'))] = key.Key('C', 'lydian') + self.assertEqual(_getKeyFromCache('C').mode, 'major') + self.assertEqual(_getKeyFromCache('c').mode, 'minor') + class TestExternal(unittest.TestCase): show = True From a5acedd11f34a2c282d01ced937350a9d17d204d Mon Sep 17 00:00:00 2001 From: Michael Scott Asato Cuthbert Date: Mon, 1 Jun 2026 19:54:29 -1000 Subject: [PATCH 2/2] remove extra AI-generated verbage. focus! focus! --- music21/roman.py | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/music21/roman.py b/music21/roman.py index 5626f0970..63360f666 100644 --- a/music21/roman.py +++ b/music21/roman.py @@ -4575,12 +4575,7 @@ def test_scale_caching(self): def test_key_cache_mode_not_just_name(self): ''' Regression test: the Key cache must distinguish modes that share a - tonic name, not just upper/lower case. Previously a `C lydian` Key - was cached under 'C' (because `tonicPitchNameWithCase` falls back to - the upper-case tonic for non-major/minor modes), so a subsequent - request for `C major` would return the cached lydian Key. - - This test was AI-assisted. + tonic name (like C major and C lydian), not just upper/lower case. ''' # lydian requested first, then major _keyCache.clear() @@ -4618,10 +4613,8 @@ def test_key_cache_mode_not_just_name(self): def test_get_key_from_cache_mode(self): ''' - `_getKeyFromCache` builds major/minor keys from string case and must - not be polluted by exotic-mode Keys stored under the bare tonic name. - - This test was AI-assisted. + Test that `_getKeyFromCache` is + not polluted by exotic-mode Keys stored under the bare tonic name. ''' _keyCache.clear() # populate the cache with a non-major/minor mode keyed on tonic 'C'