Repository navigation
(AI) preferSecondaryDominants: skip chords diatonic to the key - #2078
Conversation
romanNumeralFromChord(..., preferSecondaryDominants=True) relabelled chords that belong to the key, although the docstring says only chromatically altered chords are changed: I in major became V/IV, III in minor V/VI, and IV in minor V/VII. The substitution now only runs when the chord has a pitch outside the key's scale. In minor, the raised sixth and seventh count as diatonic. Fixes cuthbertLab#2053 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
coverage: 93.376% (+0.001%) from 93.375% — christopherlwerner:secdom-diatonic-check into cuthbertLab:master |
mscuthbert
left a comment
mscuthbert
left a comment
There was a problem hiding this comment.
Hi -- some changes requested for speed and test comprehension.
| scalePitches = keyObj.pitches | ||
| scaleNames = {p.name for p in scalePitches} | ||
| if keyObj.mode == 'minor': | ||
| for naturalSixthOrSeventh in scalePitches[5:7]: | ||
| scaleNames.add(naturalSixthOrSeventh.transpose('A1').name) | ||
| return all(p.name in scaleNames for p in chordObj.pitches) |
There was a problem hiding this comment.
Looks perfect for major -- but for minor it will slow down a lot of comparisons unnecessarily. Can you rewrite so we first check lines 1,2 and 6 of the test and then only if the first line is not true to perform the expensive transpose('A1') function possibly only if the step of 6 is in the chord and only if the 7th step is in the chord -- transposition is EXPENSIVE timewise (wish it were faster) so we should only be performing it if it's necessary to distinguish between True and False.
There was a problem hiding this comment.
Thanks! I went one step further and avoided transpose() entirely: the helper now compares each pitch with the key signature, so keyObj.pitches isn't needed either. Happy to switch to your version if you prefer.
| def testPreferSecondaryDominantsSkipsDiatonicChords(self): | ||
| # https://github.com/cuthbertLab/music21/issues/2053 | ||
| # in minor, the raised sixth and seventh count as diatonic | ||
| for chordString, keyString, figure in ( | ||
| ('C4 E4 G4', 'C', 'I'), | ||
| ('E-4 G4 B-4', 'c', 'III'), | ||
| ('F4 A4 C5', 'c', 'IV'), | ||
| ): | ||
| with self.subTest(chord=chordString, key=keyString): | ||
| rn = romanNumeralFromChord(chord.Chord(chordString), | ||
| keyString, | ||
| preferSecondaryDominants=True) | ||
| self.assertEqual(rn.figure, figure) |
There was a problem hiding this comment.
This is the only test so far of preferSecondaryDominants = True or False as far as I can tell ? So it would be good to test both ways with two figures and to see ones that change position -- this isn't enough to tell me that preferSecondaryDominants ever prefers them. Thanks!
There was a problem hiding this comment.
Done – each chord is now checked with and without preferSecondaryDominants, including two that change.
…ings Compare each pitch with the key signature instead of building the scale with keyObj.pitches and transposing the sixth and seventh; same results, about 100 times faster. The test now checks every chord with and without preferSecondaryDominants, including two chords whose figure changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mscuthbert
left a comment
mscuthbert
left a comment
There was a problem hiding this comment.
WOW! This is SOOOOOO BEAUTIFUL! Thank you! I was wondering "AITA" for pushing back at what was already a pretty good PR, but now I see -- this is incredible -- the kind of code I'll keep coming back to as "YES! This is what good code looks like"
And thanks for the I vs V/iv test -- that's really important! :-)
3 participants
Fixes #2053
preferSecondaryDominantsrelabelled chords that belong to the key (Iin majorbecame
V/IV,IIIin minorV/VI), although the docstring says only chromaticchords are changed.
The substitution now runs only when a chord has a pitch outside the key's scale.
As discussed in the issue, the raised sixth and seventh count as diatonic in minor,
so
IVin minor staysIV.Thanks for the quick feedback and the vio example in the issue!
AI-assisted (Claude)