Repository: cuthbertLab/music21 (python)

Issue:
Importing chord symbols from MusicXML with both bass and inversion tags results in the loss of the bass tag upon export to MusicXML
<!-- Please note that Michael Cuthbert, creator of music21, is taking a sabbatical from
     the project (after 17 years of continuous development, until at least January 2024. 
     The issues you raise here are unlikely to be addressed until then (unless you choose
     paid support options).  Please see the music21list Google group for mure information
     and assistance options.-->

<!-- Version of music21:  print(music21.__version__) -->
**music21 version**

`9.5.0`   <!-- replace with actual version -->


**Problem summary**
<!-- Briefly: what undesired thing happens when what action is taken? -->
When reading a chord symbol from MusicXML that contains both the `bass` tag and `inversion` tag, the bass is removed from `_overrides`. When writing it out as MusicXML again, the resulting XML file does not contain the `bass` tag but only the `inversion` tag, causing the slash chord to be lost.

I am not sure if this is intentional. I've dug into decades of music21 commit history, issues, and PRs, but couldn't find anything that explicitly state this is intended behavior.

**Steps to reproduce**

Below is a chord symbol in MusicXML that is triggering this problem:
```
      <harmony default-y="40">
        <root>
          <root-step>A</root-step>
        </root>
        <kind text="m">minor</kind>
        <inversion>1</inversion>
        <bass>
          <bass-step>C</bass-step>
        </bass>
      </harmony>
```

Just simply parsing and writing it out will cause it to lost the bass:
```
score = music21.converter.parse("cs_with_bass_and_inversion.musicxml")
score.write("musicxml", "cs_with_only_inversion.musicxml")
```

**Expected vs. actual behavior**
<!-- Consider annotating the output produced by a function -->
Not sure what the expected behavior should be. It looks like MusicXML spec doesn't say whether bass or inversion should take precedence. The chord symbol MusicXML in question was generated by Finale, and it does not always produce a slash chord MusicXML with both tags present.

**More information**
<!-- If relevant: OS, suggested fix, attempted workarounds -->
OS: macOS, Linux

<!-- Note: only Mac and Windows are directly supported by music21 staff.  
     Bug reports that are specific to other OSes w/o a PR will be closed
     unless the reporter is willing to contribute a fix.
 -->


Pull request:
Roundtrip inverted chord symbols properly
Fixes #1756

Allow ChordSymbols to specify inversion and bass and round-trip them properly.

Adds several missing tests and corner case coverage

Patch:
diff --git a/music21/_version.py b/music21/_version.py
index cabf7fb70..1f44314e2 100644
--- a/music21/_version.py
+++ b/music21/_version.py
@@ -50,7 +50,7 @@ so change it if a bug or new feature creates a problem with using old pickles.
 '''
 from __future__ import annotations
 
-__version__ = '9.6.0b3'
+__version__ = '9.6.0b4'
 
 def get_version_tuple(vv):
     v = vv.split('.')
diff --git a/music21/base.py b/music21/base.py
index 449431ee7..446b6d8e2 100644
--- a/music21/base.py
+++ b/music21/base.py
@@ -27,7 +27,7 @@ available after importing `music21`.
 <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":
diff --git a/music21/chord/__init__.py b/music21/chord/__init__.py
index 91803838c..8c6e079ef 100644
--- a/music21/chord/__init__.py
+++ b/music21/chord/__init__.py
@@ -2318,7 +2318,7 @@ class Chord(ChordBase):
         *,
         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 @@ class Chord(ChordBase):
         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 @@ class Chord(ChordBase):
         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 @@ class Chord(ChordBase):
         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)
         '''
diff --git a/music21/harmony.py b/music21/harmony.py
index 230332b65..b147fbc8c 100644
--- a/music21/harmony.py
+++ b/music21/harmony.py
@@ -263,24 +263,29 @@ class Harmony(chord.Chord):
             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):
@@ -288,13 +293,17 @@ class Harmony(chord.Chord):
             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 @@ class Test(unittest.TestCase):
         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
diff --git a/music21/musicxml/m21ToXml.py b/music21/musicxml/m21ToXml.py
index ec2441bbb..6e31ce37d 100644
--- a/music21/musicxml/m21ToXml.py
+++ b/music21/musicxml/m21ToXml.py
@@ -5951,7 +5951,6 @@ class MeasureExporter(XMLExporterBase):
         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')
@@ -5994,6 +5993,9 @@ class MeasureExporter(XMLExporterBase):
             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')
diff --git a/music21/musicxml/xmlToM21.py b/music21/musicxml/xmlToM21.py
index 18a6a8b68..c32975c30 100644
--- a/music21/musicxml/xmlToM21.py
+++ b/music21/musicxml/xmlToM21.py
@@ -5192,8 +5192,8 @@ class MeasureParser(SoundTagMixin, XMLParserBase):
         <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
@@ -5206,7 +5206,7 @@ class MeasureParser(SoundTagMixin, XMLParserBase):
 
         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')


Test patch:
diff --git a/music21/musicxml/test_m21ToXml.py b/music21/musicxml/test_m21ToXml.py
index b9cb461e3..0c5466c70 100644
--- a/music21/musicxml/test_m21ToXml.py
+++ b/music21/musicxml/test_m21ToXml.py
@@ -36,6 +36,7 @@ from music21.musicxml.m21ToXml import (
     GeneralObjectExporter, ScoreExporter,
     MusicXMLWarning, MusicXMLExportException
 )
+from music21.musicxml.xmlToM21 import MeasureParser
 
 def stripInnerSpaces(txt: str):
     '''
@@ -305,6 +306,34 @@ class Test(unittest.TestCase):
         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()