Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

romanNumeralFromChord() adds cautionary 'b' to VI and VII despite other system defaults #1217

Closed
napulen opened this issue Feb 10, 2022 · 2 comments · Fixed by #1230
Closed

Comments

@napulen
Copy link
Contributor

napulen commented Feb 10, 2022

Here is the minimal reproducibility code:

In [1]: import music21

In [2]: music21.__version__
Out[2]: '7.1.0'

In [3]: from music21.roman import romanNumeralFromChord, RomanNumeral

In [4]: from music21.chord import Chord

In [5]: c = Chord("E-4 G4 B-4")

In [6]: rn = romanNumeralFromChord(c, "f")

In [7]: rn.pitches
Out[7]:
(<music21.pitch.Pitch E-4>,
 <music21.pitch.Pitch G4>,
 <music21.pitch.Pitch B-4>)

In [8]: rn2 = RomanNumeral("bVII", "f")

In [9]: rn2.pitches
Out[9]:
(<music21.pitch.Pitch E--5>,
 <music21.pitch.Pitch G-5>,
 <music21.pitch.Pitch B--5>)

The idea here is that two different paths to obtain the RomanNumeral object lead to the same figure but different pitches.

@jacobtylerwalls
Copy link
Member

Yeah, the lack of round-trip equivalence feels surprising. Both directions are currently working as documented. The result of [8] can be changed by passing seventhMinor=roman.Minor67Default.CAUTIONARY, but since that's not the default, then I suppose [6] should put out a figure VII instead of bVII so that the round trip works by default.

This diff seems to help, but maybe the subsequent branches need to be looked at also?

diff --git a/music21/roman.py b/music21/roman.py
index 7286a7003..ae427215f 100644
--- a/music21/roman.py
+++ b/music21/roman.py
@@ -695,7 +695,7 @@ def correctRNAlterationForMinor(figureTuple, keyObj):
         rootAlterationString = ''
     elif alter == 0.0:
         alter = 0  # NB! does not change!
-        rootAlterationString = 'b'
+        rootAlterationString = ''
     # more exotic:
     elif alter > 1.0:
         alter = alter - 1

@jacobtylerwalls jacobtylerwalls changed the title Different pitches, same RomanNumeral figure romanNumeralFromChord() adds cautionary 'b' to VI and VII despite other system defaults Feb 11, 2022
@mscuthbert
Copy link
Member

There was so much discussion about ^6 and ^7 roman numerals just a few years back, so we're not going to change the defaults now. But it does seem that this is a bug. But the fix should take into account roman.Minor67Default in what it displays.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants