Merge pull request #1775 from cuthbertLab/chord_symbol_rt

Roundtrip inverted chord symbols properly

Author: mscuthbert

music21/_version.py

@@ -50,7 +50,7 @@
 '''
 from __future__ import annotations
 
-__version__ = '9.6.0b3'
+__version__ = '9.6.0b4'
 
 def get_version_tuple(vv):
     v = vv.split('.')

music21/base.py

@@ -27,7 +27,7 @@
 <class 'music21.base.Music21Object'>
 
 >>> music21.VERSION_STR
-'9.6.0b3'
+'9.6.0b4'
 
 Alternatively, after doing a complete import, these classes are available
 under the module "base":

music21/chord/__init__.py

@@ -2318,7 +2318,7 @@ def inversion(
         *,
         find: bool = True,
         testRoot: pitch.Pitch|None = None,
-        transposeOnSet: bool = True
+        transposeOnSet: bool = True,
     ) -> int|None:
         '''
         Find the chord's inversion or (if called with a number) set the chord to
@@ -2393,7 +2393,6 @@ def inversion(
         Traceback (most recent call last):
         music21.chord.ChordException: Could not invert chord: inversion may not exist
 
-
         If testRoot is True then that temporary root is used instead of self.root().
 
         Get the inversion for a seventh chord showing different roots
@@ -2430,7 +2429,7 @@ def inversion(
         sets the value to be returned later, which might be useful for
         cases where the chords are poorly spelled, or there is an added note.
 
-        * Changed in v8: chords without pitches
+        * Changed in v8: deal with chords without pitches
         '''
         if not self.pitches:
             return -1
@@ -2460,11 +2459,12 @@ def inversion(
         else:
             return -1
 
-    def _setInversion(self,
-                      newInversion: int,
-                      rootPitch: pitch.Pitch,
-                      transposeOnSet: bool
-                      ) -> None:
+    def _setInversion(
+        self,
+        newInversion: int,
+        rootPitch: pitch.Pitch,
+        transposeOnSet: bool,
+    ) -> None:
         '''
         Helper function for inversion(int)
         '''

music21/harmony.py

@@ -263,38 +263,47 @@ def _reprInternal(self):
             summary += ' '.join([p.name for p in self.pitches])
         return summary
 
-    # PRIVATE METHODS #
+    # PROTECTED METHODS #
     def _parseFigure(self):
         '''
-        subclass this in extensions (SO WHY IS IT PRIVATE???)
+        subclass this in extensions (protected in TypeScript/Java speak)
         '''
         return
 
     def _updatePitches(self):
         '''
-        subclass this in extensions (SO WHY IS IT PRIVATE???)
+        subclass this in extensions (protected in TypeScript/Java speak)
         '''
         return
 
-    def _updateFromParameters(self, root, bass, inversion: int|None = None):
+    def _updateFromParameters(
+        self,
+        root: str|pitch.Pitch|None,
+        bass: str|pitch.Pitch|None,
+        inversion: int|None = None
+    ) -> None:
         '''
         This method must be called twice, once before the pitches
         are rendered, and once after. This is because after the pitches
-        are rendered, the root() and bass() becomes reset by the chord class,
+        are rendered, the root() and bass() become reset by the chord class,
         but we want the objects to retain their initial root, bass, and inversion.
         '''
         if root and isinstance(root, str):
             root = common.cleanedFlatNotation(root)
             self.root(pitch.Pitch(root, octave=3))
         elif root is not None:
             self.root(root)
+
+        # set inversion first...
+        if inversion is not None:
+            self.inversion(inversion, transposeOnSet=True)
+
+        # and then bass.
         if bass and isinstance(bass, str):
             bass = common.cleanedFlatNotation(bass)
             self.bass(pitch.Pitch(bass, octave=3), allow_add=True)
         elif bass is not None:
             self.bass(bass, allow_add=True)
-        if inversion is not None:
-            self.inversion(inversion, transposeOnSet=True)
 
     # PUBLIC PROPERTIES #
 
@@ -1609,7 +1618,7 @@ class ChordSymbol(Harmony):
     # INITIALIZER #
 
     def __init__(self,
-                 figure=None,
+                 figure: str|None = None,
                  root: pitch.Pitch|str|None = None,
                  bass: pitch.Pitch|str|None = None,
                  inversion: int|None = None,
@@ -2473,7 +2482,13 @@ class NoChord(ChordSymbol):
     >>> nc2.pitches
     ()
     '''
-    def __init__(self, figure=None, kind='none', kindStr=None, **keywords):
+    def __init__(
+        self,
+        figure: str|None = None,
+        kind: str|None = 'none',
+        kindStr: str|None = None,
+        **keywords
+    ):
         super().__init__(figure, kind=kind, kindStr=kindStr or figure or 'N.C.', **keywords)
 
         if self._figure is None:
@@ -2655,6 +2670,17 @@ def testChordSymbolSetsBassOctave(self):
         b = d.bass()
         self.assertEqual(b.nameWithOctave, 'E-3')
 
+    def testHarmonyPreservesInversionAndBass(self):
+        '''
+        Test that bass is preserved even when both bass and inversion are given
+        '''
+        explicitFm6 = ChordSymbol(root='F', bass='A-', inversion=1, kind='minor')
+        self.assertEqual(explicitFm6.inversion(), 1)
+        self.assertEqual(explicitFm6.bass(find=False).name, 'A-')
+        self.assertEqual(explicitFm6.root(find=False).name, 'F')
+        self.assertLess(explicitFm6.bass(find=False).octave,
+                        explicitFm6.root(find=False).octave)
+
     def testClassSortOrderHarmony(self):
         '''
         This tests a former bug in getContextByClass

music21/musicxml/m21ToXml.py

@@ -6022,7 +6022,6 @@ def chordSymbolToXml(self,
         self.setPrintStyle(mxHarmony, cs)
 
         csRoot = cs.root()
-        csBass = cs.bass(find=False)
         # TODO: do not look at ._attributes
         if cs._roman is not None:
             mxFunction = SubElement(mxHarmony, 'function')
@@ -6065,6 +6064,9 @@ def chordSymbolToXml(self,
             mxInversion = SubElement(mxHarmony, 'inversion')
             mxInversion.text = str(csInv)
 
+        # first -- go with the already defined bass, from overrides, etc.
+        # but if that is not defined, then find the bass itself.
+        csBass = cs.bass(find=False) or cs.bass(find=True)
         if csBass is not None and (csRoot is None or csRoot.name != csBass.name):
             # TODO.. reuse above from Root
             mxBass = SubElement(mxHarmony, 'bass')

music21/musicxml/test_m21ToXml.py

@@ -36,6 +36,7 @@
     GeneralObjectExporter, ScoreExporter,
     MusicXMLWarning, MusicXMLExportException
 )
+from music21.musicxml.xmlToM21 import MeasureParser
 
 def stripInnerSpaces(txt: str):
     '''
@@ -305,6 +306,34 @@ def testCompositeLyrics(self):
         self.assertEqual(ly2.findall('syllabic')[1].text, 'end')
         self.assertEqual(ly2.findall('text')[1].text, 'e')
 
+    def testExportChordSymbolWithInversions(self):
+        '''
+        This test checks for the issue in 1756 where a chord symbol imported
+        with both inversion and bass would not have them both set.
+
+        There were multiple bugs fixed, so important to check.
+        '''
+        explicitFm6 = harmony.ChordSymbol(root='F', bass='A-', inversion=1, kind='minor')
+        et = self.getET(explicitFm6)
+        harmonyEls = et.findall('part/measure/harmony')
+        self.assertEqual(len(harmonyEls), 1)
+        harmonyEl = harmonyEls[0]
+        # helpers.dump(harmonyEl)
+        root = harmonyEl.find('root')
+        rootStep = root.find('root-step')
+        self.assertEqual(rootStep.text, 'F')
+        self.assertEqual(harmonyEl.find('kind').text, 'minor')
+        self.assertEqual(harmonyEl.find('inversion').text, '1')
+        self.assertEqual(harmonyEl.find('bass/bass-step').text, 'A')
+        self.assertEqual(harmonyEl.find('bass/bass-alter').text, '-1')
+
+        # test round trip
+        mp = MeasureParser()
+        cs = mp.xmlToChordSymbol(harmonyEl)
+        self.assertEqual(explicitFm6.bass(find=False), cs.bass(find=False))
+        self.assertEqual(explicitFm6.root(find=False), cs.root(find=False))
+        self.assertEqual(explicitFm6.inversion(), cs.inversion())
+
     def testExportNC(self):
         s = stream.Score()
         p = stream.Part()

music21/musicxml/xmlToM21.py

@@ -5252,8 +5252,8 @@ def xmlToChordSymbol(
         <music21.pitch.Pitch D-3>
         '''
         # TODO: musicxml 4: attr: arrangement -- C/E or C over E etc.
-        # TODO: offset
-        # Element staff is covered by insertCoreAndReference in xmlHarmony()
+        # Element offset is covered by xmlHarmony(), which calls this.
+        # Element staff is also covered by insertCoreAndReference in xmlHarmony()
         b: pitch.Pitch|None = None
         r: pitch.Pitch|None = None
         inversion: int|None = None
@@ -5266,7 +5266,7 @@ def xmlToChordSymbol(
 
         mxFrame = mxHarmony.find('frame')
 
-        mxBass = mxHarmony.find('bass')
+        mxBass: ET.Element | None = mxHarmony.find('bass')
         if mxBass is not None:
             # required
             bassStep = mxBass.find('bass-step')