Repository: cuthbertLab/music21
Pull request #1323 (2022-06-06 20:02:50)

Fix a regression with chord accidentals
Better solution than #1299. That PR caused a regression with this case:

```python
from music21 import chord
c = chord.Chord("D# F# A#")
c.show(app='Finale')
```
<img width="121" alt="Screen Shot 2022-06-06 at 4 01 46 PM" src="https://user-images.githubusercontent.com/38668450/172238690-8b514a31-9f99-426f-b5d1-3119eec9b281.png">

***
A better solution is to actually distinguish "past" from "simultaneous" rather than mixing the two notions.


mscuthbert: Thamks for the great catch -- feel free to merge after the `test_pitch.py` failure is updated to use a keyword arg.
coveralls: 
[![Coverage Status](https://coveralls.io/builds/49774593/badge)](https://coveralls.io/builds/49774593)

Coverage increased (+0.0009%) to 93.054% when pulling **7df002a0c9c008597cd05d59670545e70577f436 on jacobtylerwalls:chord-accidentals-regression** into **9f2143c00e12b307d775d57d51a5493e0c7e1931 on cuthbertLab:master**.


Patch:
diff --git a/music21/pitch.py b/music21/pitch.py
index 3f93fe6ef..fe724afc2 100644
--- a/music21/pitch.py
+++ b/music21/pitch.py
@@ -4733,8 +4733,10 @@ class Pitch(prebase.ProtoM21Object):
 
     def updateAccidentalDisplay(
         self,
+        *,
         pitchPast: t.Optional[t.List['Pitch']] = None,
         pitchPastMeasure: t.Optional[t.List['Pitch']] = None,
+        otherSimultaneousPitches: t.Optional[t.List['Pitch']] = None,
         alteredPitches: t.Optional[t.List['Pitch']] = None,
         cautionaryPitchClass: bool = True,
         cautionaryAll: bool = False,
@@ -4755,6 +4757,9 @@ class Pitch(prebase.ProtoM21Object):
         `pitchPastMeasure` is a list of pitches preceding this pitch but in a
         previous measure. If None, a new list will be made.
 
+        `otherSimultaneousPitches` is a list of other pitches in this simultaneity, for use
+        when `cautionaryPitchClass` is True.
+
         The `alteredPitches` list supplies pitches from a :class:`~music21.key.KeySignature`
         object using the :attr:`~music21.key.KeySignature.alteredPitches` property.
         If None, a new list will be made.
@@ -4784,13 +4789,13 @@ class Pitch(prebase.ProtoM21Object):
 
         >>> a = pitch.Pitch('a')
         >>> past = [pitch.Pitch('a#'), pitch.Pitch('c#'), pitch.Pitch('c')]
-        >>> a.updateAccidentalDisplay(past, cautionaryAll=True)
+        >>> a.updateAccidentalDisplay(pitchPast=past, cautionaryAll=True)
         >>> a.accidental, a.accidental.displayStatus
         (<music21.pitch.Accidental natural>, True)
 
         >>> b = pitch.Pitch('a')
         >>> past = [pitch.Pitch('a#'), pitch.Pitch('c#'), pitch.Pitch('c')]
-        >>> b.updateAccidentalDisplay(past)  # should add a natural
+        >>> b.updateAccidentalDisplay(pitchPast=past)  # should add a natural
         >>> b.accidental, b.accidental.displayStatus
         (<music21.pitch.Accidental natural>, True)
 
@@ -4799,10 +4804,11 @@ class Pitch(prebase.ProtoM21Object):
 
         >>> a4 = pitch.Pitch('a4')
         >>> past = [pitch.Pitch('a#3'), pitch.Pitch('c#'), pitch.Pitch('c')]
-        >>> a4.updateAccidentalDisplay(past, cautionaryPitchClass=False)
+        >>> a4.updateAccidentalDisplay(pitchPast=past, cautionaryPitchClass=False)
         >>> a4.accidental is None
         True
 
+        v8 -- made keyword-only and added `otherSimultaneousPitches`.
         '''
         # N.B. -- this is a very complex method
         # do not alter it without significant testing.
@@ -4827,10 +4833,6 @@ class Pitch(prebase.ProtoM21Object):
         if alteredPitches is None:
             alteredPitches = []
 
-        # TODO: this presently deals with chords as simply a list
-        #    we might permit pitchPast to contain a list of pitches, to represent
-        #    a simultaneity?
-
         # should we display accidental if no previous accidentals have been displayed
         # i.e. if it's the first instance of an accidental after a tie
         displayAccidentalIfNoPreviousAccidentals = False
@@ -4861,6 +4863,15 @@ class Pitch(prebase.ProtoM21Object):
             else:
                 return  # exit: nothing more to do
 
+        if (
+            otherSimultaneousPitches
+            and cautionaryPitchClass
+            and any(pSimult.step == self.step and pSimult.pitchClass != self.pitchClass
+                for pSimult in otherSimultaneousPitches)
+        ):
+            set_displayStatus(True)
+            return
+
         # no pitches in past...
         if not pitchPastAll:
             # if we have no past, we show the accidental if this pitch name
@@ -5061,13 +5072,6 @@ class Pitch(prebase.ProtoM21Object):
                       and pPastInMeasure is False):
                     set_displayStatus(True)
 
-                # Avoid making early, incorrect assumptions if this pitch is part of a chord.
-                # pPast might include this note itself, in which case we should not say
-                # "natural already in past usage". (Filtering out the current note from pPast
-                # when calling this method is not sufficient, because there could be repetitions.)
-                elif self._client is not None and self._client._chordAttached is not None:
-                    continue
-
                 # other cases: already natural in past usage, do not need
                 # natural again (and not in key sig)
                 else:
diff --git a/music21/stream/base.py b/music21/stream/base.py
index 1897ae514..a7f8a090e 100644
--- a/music21/stream/base.py
+++ b/music21/stream/base.py
@@ -6319,6 +6319,7 @@ class Stream(core.StreamCore, t.Generic[M21ObjType]):
         *,
         pitchPast: t.Optional[t.List[pitch.Pitch]] = None,
         pitchPastMeasure: t.Optional[t.List[pitch.Pitch]] = None,
+        otherSimultaneousPitches: t.Optional[t.List[pitch.Pitch]] = None,
         useKeySignature: t.Union[bool, key.KeySignature] = True,
         alteredPitches: t.Optional[t.List[pitch.Pitch]] = None,
         searchKeySignatureByContext: bool = False,
@@ -6336,6 +6337,8 @@ class Stream(core.StreamCore, t.Generic[M21ObjType]):
 
         `pitchPastMeasure` is a list of pitches preceding this pitch but in a previous measure.
 
+        `otherSimultaneousPitches` is a list of other pitches in this simultaneity, for use
+        when `cautionaryPitchClass` is True.
 
         If `useKeySignature` is True, a :class:`~music21.key.KeySignature` will be searched
         for in this Stream or this Stream's defined contexts. An alternative KeySignature
@@ -6377,7 +6380,7 @@ class Stream(core.StreamCore, t.Generic[M21ObjType]):
 
         Changed in v.6: does not return anything if inPlace is True.
         Changed in v.7: default inPlace is False
-        Changed in v.8: altered unisons in Chords now supply clarifying naturals.
+        Changed in v.8: altered unisons/octaves in Chords now supply clarifying naturals.
 
         All arguments are keyword only.
         '''
@@ -6459,11 +6462,9 @@ class Stream(core.StreamCore, t.Generic[M21ObjType]):
                     tiePitchSet.add(e.pitch.nameWithOctave)
 
             elif isinstance(e, chord.Chord):
-                # add all chord elements to past first
                 # when reading a chord, this will apply an accidental
                 # if pitches in the chord suggest an accidental
                 seenPitchNames = set()
-                pitchPast += e.pitches
 
                 for n in list(e):
                     p = n.pitch
@@ -6472,9 +6473,12 @@ class Stream(core.StreamCore, t.Generic[M21ObjType]):
                     else:
                         lastNoteWasTied = False
 
+                    otherSimultaneousPitches = [other for other in e.pitches if other is not p]
+
                     p.updateAccidentalDisplay(
                         pitchPast=pitchPast,
                         pitchPastMeasure=pitchPastMeasure,
+                        otherSimultaneousPitches=otherSimultaneousPitches,
                         alteredPitches=alteredPitches,
                         cautionaryPitchClass=cautionaryPitchClass,
                         cautionaryAll=cautionaryAll,
@@ -6489,6 +6493,7 @@ class Stream(core.StreamCore, t.Generic[M21ObjType]):
                 for pName in seenPitchNames:
                     tiePitchSet.add(pName)
 
+                pitchPast += e.pitches
             else:
                 tiePitchSet.clear()
 


Test patch:
diff --git a/music21/stream/tests.py b/music21/stream/tests.py
index 40d95fbcb..35f8b7bc0 100644
--- a/music21/stream/tests.py
+++ b/music21/stream/tests.py
@@ -2613,10 +2613,8 @@ class Test(unittest.TestCase):
         self.assertTrue(c1.pitches[1].accidental.displayStatus)
         self.assertTrue(c1.pitches[2].accidental.displayStatus)
 
-        # not getting a natural here because of chord tones
-        # self.assertTrue(n3.pitch.accidental.displayStatus)
-        # self.assertEqual(n3.pitch.accidental, None)
-        # s.show()
+        # not necessary to repeat the natural afterward
+        self.assertIsNone(n3.pitch.accidental, None)
 
         s = Stream()
         n1 = note.Note('a#')
@@ -2794,6 +2792,35 @@ class Test(unittest.TestCase):
 
         # TODO: other types
 
+    def testMakeAccidentalsOnChord(self):
+        c = chord.Chord('F# A# C#')
+        s = Stream(c)
+        self.assertFalse(any(n.pitch.accidental.displayStatus for n in c))
+        s.makeAccidentals(inPlace=True)
+        self.assertTrue(all(n.pitch.accidental.displayStatus for n in c))
+
+        augmented_octave = chord.Chord('F4 F#5')
+        s2 = Stream(augmented_octave)
+        self.assertIsNone(augmented_octave[0].pitch.accidental)
+        s2.makeAccidentals(inPlace=True)
+        self.assertTrue(augmented_octave[0].pitch.accidental.displayStatus)
+
+        # Repeat the test without octaves and reset state
+        low, high = augmented_octave.pitches
+        low.octave = None
+        low.accidental = None
+        high.octave = None
+        high.accidental.displayStatus = None
+
+        s2.makeAccidentals(inPlace=True)
+        self.assertTrue(all(n.pitch.accidental.displayStatus for n in augmented_octave))
+
+        # Repeat the test with two F#'s in key of 1 sharp
+        two_f_sharps = chord.Chord('F#4 F#5')
+        s3 = Stream([key.KeySignature(1), two_f_sharps])
+        s3.makeAccidentals(inPlace=True)
+        self.assertTrue(all(n.pitch.accidental.displayStatus is False for n in two_f_sharps))
+
     def testMakeNotationTiesKeyless(self):
         p = converter.parse('tinynotation: 4/4 f#1~ f#1')
         # Key of no sharps/flats
diff --git a/music21/test/test_pitch.py b/music21/test/test_pitch.py
index 7247ab797..888d34b0b 100644
--- a/music21/test/test_pitch.py
+++ b/music21/test/test_pitch.py
@@ -89,7 +89,7 @@ class Test(unittest.TestCase):
         self.assertEqual(a.name, 'C')
         self.assertTrue(a.accidental.displayStatus)
 
-        a.updateAccidentalDisplay(past, overrideStatus=True)
+        a.updateAccidentalDisplay(pitchPast=past, overrideStatus=True)
         self.assertFalse(a.accidental.displayStatus)
 
         b = copy.deepcopy(a)
@@ -101,7 +101,7 @@ class Test(unittest.TestCase):
         '''
         def proc(_pList, past):
             for p in _pList:
-                p.updateAccidentalDisplay(past)
+                p.updateAccidentalDisplay(pitchPast=past)
                 past.append(p)
 
         def compare(past, _result):
@@ -206,7 +206,7 @@ class Test(unittest.TestCase):
         '''
         def proc(_pList, past, alteredPitches):
             for p in _pList:
-                p.updateAccidentalDisplay(past, alteredPitches=alteredPitches)
+                p.updateAccidentalDisplay(pitchPast=past, alteredPitches=alteredPitches)
                 past.append(p)
 
         def compare(past, _result):
@@ -321,13 +321,13 @@ class Test(unittest.TestCase):
         '''
         def proc1(_pList, _past):
             for p in _pList:
-                p.updateAccidentalDisplay(_past, cautionaryPitchClass=True,
+                p.updateAccidentalDisplay(pitchPast=_past, cautionaryPitchClass=True,
                                           cautionaryNotImmediateRepeat=False)
                 _past.append(p)
 
         def proc2(_pList, _past):
             for p in _pList:
-                p.updateAccidentalDisplay(_past, cautionaryPitchClass=False,
+                p.updateAccidentalDisplay(pitchPast=_past, cautionaryPitchClass=False,
                                           cautionaryNotImmediateRepeat=False)
                 _past.append(p)
 
@@ -366,7 +366,7 @@ class Test(unittest.TestCase):
         a4 = Pitch('a4')
         past = [Pitch('a#3'), Pitch('c#'), Pitch('c')]
         # will not add a natural because match is pitchSpace
-        a4.updateAccidentalDisplay(past, cautionaryPitchClass=False)
+        a4.updateAccidentalDisplay(pitchPast=past, cautionaryPitchClass=False)
         self.assertEqual(a4.accidental, None)
 
     def testAccidentalsCautionary(self):