Fix Raised 6, 7 issues with obscure RNs (Issue 1349) - #2001
Merged
Conversation
Major-quality chords on the raised sixth and seventh scale degrees of a minor key now keep their sharp prefix (e.g. #VI in c minor instead of VI#63), so the generated figure round-trips through RomanNumeral() and .romanNumeral distinguishes them from the chords on the lowered (natural minor) degrees. Adds the keyword-only isMajorThird argument to correctRNAlterationForMinor, documents the semantic contract of the .romanNumeral property, and removes the now-unneeded workaround in romanText/tsvConverter.localKeyAsRn. Addresses the main bug in #1349 (the sixthMinor/seventhMinor parameters for romanNumeralFromChord and the QUALITY default flip remain open). AI-assisted (Claude)
Documents the previously implicit semantics: aboveBass is reckoned from whatever reference pitch was passed (the tonic, not the sounding bass, in romanNumeralFromChord); alter is measured against the natural-minor scale; and prefix deliberately diverges from alter after correctRNAlterationForMinor applies cautionary conventions. Also adds a maintenance note listing the three places that implement the minor ^6/^7 accidental convention. No behavior change. AI-assisted (Claude)
aboveBass becomes degFromRefPitch: the reference pitch is not always the bass (romanNumeralFromChord passes the tonic, measuring the chord's root), and the value is a 1-based simple-interval degree, not a zero-based distance above. ChordFigureTuple becomes PitchFigureTuple: one tuple describes a single pitch; a list of them describes a chord. Renames the matching local variable in figureTupleSolo and updates all docs and doctests. AI-assisted (Claude)
Public docstrings are for library users; maintenance notes and references to private methods belong in comments for maintainers. AI-assisted (Claude)
…eral The function corrects Roman-numeral alterations, so its parameter should speak in numeral terms, not chord anatomy: what it consumes is whether the numeral will be written in uppercase (lowercase numerals already imply the raised ^6/^7 in minor). Major third and uppercase numeral are the same bit; romanNumeralFromChord translates at the call site. AI-assisted (Claude)
Checks session-learned working knowledge into the repo-shared memory so it survives across machines: public docs vs maintainer comments, no untaken-paths prose in docstrings, and the status/open questions on the romanNumeral raised-6/7 work. AI-assisted (Claude)
…e doc The variable names a fact about the chord, not about any single interval or figure entry. Also cut the not-a-subclass parenthetical from the PitchFigureTuple docstring. AI-assisted (Claude)
Name the parameter for the chord-level fact it carries, not for its consequence: no numeral exists yet when the function runs, so uppercaseNumeral described something outside its inputs. One concept now has one name in caller and callee alike; the docstring explains the case convention that makes the fact matter. AI-assisted (Claude)
The variable is a plain bool (semitonesFromChordStep returns None for third-less chords, which the == 4 comparison maps to False), so the five-line assignment collapses to one and the 'if X: pass / elif not X' becomes a single negated if. AI-assisted (Claude)
Follows the module's result-From-source naming (romanNumeralFromChord); the old name read as the verb 'figure'. The new function's reference-pitch parameter is refPitch (was bass), matching FigureTuple.degFromRefPitch and the fact that romanNumeralFromChord passes the tonic, not a bass. figureTupleSolo remains at its old spot as a deprecated (v11 -> v12) alias with the old signature. AI-assisted (Claude)
… name" This reverts commit 14fe359.
…ethod The measurement that produces a FigureTuple now lives on the type it produces, with the reference-pitch parameter named refPitch to match degFromRefPitch. figureTupleSolo stays at its old spot as a deprecated (v11 -> v12) alias with its old signature. AI-assisted (Claude)
add editorconfig
|
coverage: 93.33%. remained the same — fix-1349-raised-67 into master |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the main bug in #1349 :
romanNumeralFromChordnow keeps the sharp onmajor-quality chords built on raised ^6/^7 in minor (
#VI, notVI#63),So more figures round-trip accurately. Improve and document
.romanNumeraland howit distinguishes them from
bVI/bVII.Removes the tsvConverter workaround for this bug since now it just works.
Documents the long-implicit inner workings of
.romanNumeral(natural-minor spelling)and the various figure tuples:
FigureTuple/PitchFigureTuplewith.fromPitchAndReference()class methodsDeprecate
figureTupleSolo.Add
.editorconfigfor guide lines for ruff. (someone complain if you hate them, but i do most of the dev here...) Add some AI memories about how to write docs.AI-Assisted (Claude) - thoroughly human reviewed and prose rewritten
Fixes #1349