Repository: cuthbertLab/music21 (python)

Issue:
Accidental reiterated on tied chromatic pitch if key signature present
**music21 version**

7


**Problem summary**
A tied pitch has its accidental unnecessarily reiterated when running `makeNotation` if 
the key is no sharps or flats
 there is a key object somewhere.

**Steps to reproduce**

```
from music21 import converter
p = converter.parse('tinynotation: 4/4 f#1~ f#1')
p.measure(1).insert(0, key.KeySignature(sharps=0))
# makeNotation performs some checks before calling makeAccidentals
p.makeNotation(inPlace=True)
p.show()
```

![image](https://user-images.githubusercontent.com/38668450/108523871-6af77900-729c-11eb-9f63-103899ab3b1e.png)

**More information**

Noticed this in the User's Guide, the last example in Ch. 9 shows an image of this working correctly (based on the series of calls to chordify, etc.), but it no longer works.

Pull request:
Avoid reiterating accidentals on diatonic or tied pitches in excerpts with key signatures
Fixes #865 

Patch:
diff --git a/documentation/source/usersGuide/usersGuide_30_examples3.ipynb b/documentation/source/usersGuide/usersGuide_30_examples3.ipynb
index e900f1ee5..e21c40eca 100644
--- a/documentation/source/usersGuide/usersGuide_30_examples3.ipynb
+++ b/documentation/source/usersGuide/usersGuide_30_examples3.ipynb
@@ -832,7 +832,7 @@
    "cell_type": "markdown",
    "metadata": {},
    "source": [
-    "This is looking pretty decent, but have no fears, we'll find more sophisticated score reduction toos in `music21` in the chapters to come."
+    "This is looking pretty decent, but have no fears, we'll find more sophisticated score reduction tools in `music21` in the chapters to come."
    ]
   },
   {
diff --git a/documentation/source/usersGuide/usersGuide_46_streams3.ipynb b/documentation/source/usersGuide/usersGuide_46_streams3.ipynb
index 7493ff7c8..a53b24f06 100644
--- a/documentation/source/usersGuide/usersGuide_46_streams3.ipynb
+++ b/documentation/source/usersGuide/usersGuide_46_streams3.ipynb
@@ -39,7 +39,7 @@
     "            continue # Rests, Chords, etc.\n",
     "        n.activeSite.replace(n, interval.Interval(n, nNext))\n",
     "    for other in out.recurse().getElementsNotOfClass(['Interval', 'Stream']):\n",
-    "        other.activeSite.remove(other)  # get rid of everyhing that is not an Interval or Stream\n",
+    "        other.activeSite.remove(other)  # get rid of everything that is not an Interval or Stream\n",
     "    return out"
    ]
   },
diff --git a/music21/pitch.py b/music21/pitch.py
index de3d437c8..a2738a922 100644
--- a/music21/pitch.py
+++ b/music21/pitch.py
@@ -4717,7 +4717,7 @@ class Pitch(prebase.ProtoM21Object):
         # store if a match was found and display set from past pitches
         setFromPitchPast = False
 
-        if cautionaryPitchClass is True:  # warn no mater what octave; thus create new without oct
+        if cautionaryPitchClass is True:  # warn no matter what octave; thus create new without oct
             pSelf = Pitch(self.name)
             pSelf.accidental = self.accidental
         else:
diff --git a/music21/stream/base.py b/music21/stream/base.py
index 65ac8ff80..e9215f9bf 100644
--- a/music21/stream/base.py
+++ b/music21/stream/base.py
@@ -6475,7 +6475,7 @@ class Stream(core.StreamCoreMixin, base.Music21Object):
 
         If `cautionaryAll` is True, all accidentals are shown.
 
-        If `overrideStatus` is True, this method will ignore any current `displayStatus` stetting
+        If `overrideStatus` is True, this method will ignore any current `displayStatus` setting
         found on the Accidental. By default this does not happen. If `displayStatus` is set to
         None, the Accidental's `displayStatus` is set.
 
@@ -6613,11 +6613,21 @@ class Stream(core.StreamCoreMixin, base.Music21Object):
         return self.streamStatus.haveAccidentalsBeenMade()
 
     def makeNotation(self,
+                     *,
                      meterStream=None,
                      refStreamOrTimeRange=None,
                      inPlace=False,
                      bestClef=False,
-                     **subroutineKeywords):
+                     pitchPast: Optional[List[pitch.Pitch]] = None,
+                     pitchPastMeasure: Optional[List[pitch.Pitch]] = None,
+                     useKeySignature: Union[bool, key.KeySignature] = True,
+                     alteredPitches: Optional[List[pitch.Pitch]] = None,
+                     cautionaryPitchClass: bool = True,
+                     cautionaryAll: bool = False,
+                     overrideStatus: bool = False,
+                     cautionaryNotImmediateRepeat: bool = True,
+                     tiePitchSet: Optional[Set[str]] = None
+                     ):
         '''
         This method calls a sequence of Stream methods on this Stream to prepare
         notation, including creating voices for overlapped regions, Measures
@@ -6626,8 +6636,18 @@ class Stream(core.StreamCoreMixin, base.Music21Object):
         If `inPlace` is True, this is done in-place (changed in v7 -- returns None);
         if `inPlace` is False, this returns a modified deep copy.
 
-        makeAccidentalsKeywords can be a dict specifying additional
-        parameters to send to makeAccidentals
+        The following additional parameters are documented on
+        :meth:`~music21.stream.base.makeAccidentals`::
+
+            pitchPast
+            pitchPastMeasure
+            useKeySignature
+            alteredPitches
+            cautionaryPitchClass
+            cautionaryAll
+            overrideStatus
+            cautionaryNotImmediateRepeat
+            tiePitchSet
 
 
         >>> s = stream.Stream()
@@ -6681,36 +6701,17 @@ class Stream(core.StreamCoreMixin, base.Music21Object):
         # this needs to be called before makeTies
         # note that this functionality is also placed in Part
         if not measureStream.haveAccidentalsBeenMade():
-            ksLast = None
-            srkCopy = subroutineKeywords.copy()
-            if 'meterStream' in srkCopy:
-                del(srkCopy['meterStream'])
-            if 'refStreamOrTimeRange' in srkCopy:
-                del(srkCopy['refStreamOrTimeRange'])
-
-            for i in range(len(measureStream)):
-                m = measureStream[i]
-                if m.keySignature is not None:
-                    ksLast = m.keySignature
-
-                if i > 0 and ksLast is None:
-                    try:
-                        previousNoteOrChord = measureStream[i - 1][-1]
-                        tiePitchSet = makeNotation.getTiePitchSet(previousNoteOrChord)
-                    except (IndexError, StreamException):
-                        tiePitchSet = None
-                    pitchPastMeasure = measureStream[i - 1].pitches
-                else:
-                    tiePitchSet = None
-                    pitchPastMeasure = None
-
-                m.makeAccidentals(
-                    pitchPastMeasure=pitchPastMeasure,
-                    useKeySignature=ksLast,
-                    searchKeySignatureByContext=False,
-                    tiePitchSet=tiePitchSet,
-                    inPlace=True,
-                    **srkCopy)
+            makeNotation.makeAccidentalsInMeasureStream(
+                measureStream,
+                pitchPast=pitchPast,
+                pitchPastMeasure=pitchPastMeasure,
+                useKeySignature=useKeySignature,
+                alteredPitches=alteredPitches,
+                cautionaryPitchClass=cautionaryPitchClass,
+                cautionaryAll=cautionaryAll,
+                overrideStatus=overrideStatus,
+                cautionaryNotImmediateRepeat=cautionaryNotImmediateRepeat,
+                tiePitchSet=tiePitchSet)
 
         measureStream.makeTies(meterStream, inPlace=True)
 
@@ -7430,7 +7431,7 @@ class Stream(core.StreamCoreMixin, base.Music21Object):
 
         When, as is commonly the case, we want to find all of the notes,
         but do not care to have offsets related to the origin of the stream,
-        then `.recurse()` is generally a more efficiant way of working:
+        then `.recurse()` is generally a more efficient way of working:
 
         >>> len(bwv66.recurse().notes)
         165
@@ -13139,39 +13140,16 @@ class Part(Stream):
         else:
             returnObj = self
         # process make accidentals for each measure
-        measureStream = returnObj.getElementsByClass('Measure')
-        ksLast = None
-        for i in range(len(measureStream)):
-            m = measureStream[i]
-            if m.keySignature is not None:
-                ksLast = m.keySignature
-            # if beyond the first measure, use the pitches from the last
-            # measure for context
-            if i > 0:
-                try:
-                    previousNoteOrChord = measureStream[i - 1][-1]
-                    tiePitchSet = makeNotation.getTiePitchSet(previousNoteOrChord)
-                except (IndexError, StreamException):
-                    tiePitchSet = None
-
-                pitchPastMeasure = measureStream[i - 1].pitches
-
-            else:
-                pitchPastMeasure = None
-                # use the tie pitchSet from the method argument
-
-            m.makeAccidentals(
-                pitchPastMeasure=pitchPastMeasure,
-                useKeySignature=ksLast,
-                alteredPitches=alteredPitches,
-                searchKeySignatureByContext=False,
-                cautionaryPitchClass=cautionaryPitchClass,
-                cautionaryAll=cautionaryAll,
-                inPlace=True,  # always, has have a copy or source
-                overrideStatus=overrideStatus,
-                cautionaryNotImmediateRepeat=cautionaryNotImmediateRepeat,
-                tiePitchSet=tiePitchSet,
-            )
+        measureStream = returnObj.getElementsByClass(Measure)
+        makeNotation.makeAccidentalsInMeasureStream(
+            measureStream,
+            alteredPitches=alteredPitches,
+            cautionaryPitchClass=cautionaryPitchClass,
+            cautionaryAll=cautionaryAll,
+            overrideStatus=overrideStatus,
+            cautionaryNotImmediateRepeat=cautionaryNotImmediateRepeat,
+            tiePitchSet=tiePitchSet,
+        )
         if not inPlace:
             return returnObj
         else:  # in place
diff --git a/music21/stream/makeNotation.py b/music21/stream/makeNotation.py
index bded35368..f0375e5cb 100644
--- a/music21/stream/makeNotation.py
+++ b/music21/stream/makeNotation.py
@@ -14,19 +14,22 @@
 
 import copy
 import unittest
-from typing import List, Generator, Optional
+from typing import List, Generator, Optional, Set, Union
 
 from music21 import beam
 from music21 import clef
 from music21 import common
 from music21 import defaults
 from music21 import environment
+from music21 import key
 from music21 import meter
 from music21 import note
 from music21 import pitch
 
 from music21.common.numberTools import opFrac
 
+from music21.exceptions21 import StreamException
+
 environLocal = environment.Environment(__file__)
 
 
@@ -1612,6 +1615,93 @@ def getTiePitchSet(prior):
             tiePitchSet.add(n.pitch.nameWithOctave)
         return tiePitchSet
 
+def makeAccidentalsInMeasureStream(
+    s: 'music21.stream.Stream',
+    *,
+    pitchPast: Optional[List[pitch.Pitch]] = None,
+    pitchPastMeasure: Optional[List[pitch.Pitch]] = None,
+    useKeySignature: Union[bool, key.KeySignature] = True,
+    alteredPitches: Optional[List[pitch.Pitch]] = None,
+    cautionaryPitchClass: bool = True,
+    cautionaryAll: bool = False,
+    overrideStatus: bool = False,
+    cautionaryNotImmediateRepeat: bool = True,
+    tiePitchSet: Optional[Set[str]] = None
+):
+    '''
+    Makes accidentals in place on a stream consisting of only Measures.
+    Helper for Stream.makeNotation and Part.makeAccidentals.
+
+    Walks measures in order to update the values for the following keyword
+    arguments of :meth:`~music21.stream.base.makeAccidentals` and calls
+    that method on each Measure. (For this reason, the values supplied
+    for these arguments in the method signature will be used on the first
+    measure only, or in the case of `useKeySignature`, not at all if the first
+    measure contains a `KeySignature`.)::
+
+        pitchPastMeasure
+        useKeySignature
+        tiePitchSet
+
+    Operates on the measures in place; make a copy first if this is not desired.
+    '''
+    if s.getElementsNotOfClass('Measure'):
+        raise ValueError(f'{s} must contain only Measures')
+
+    # bool values for useKeySignature are not helpful here
+    # because we are definitely searching key signature contexts
+    # only key.KeySignature values are interesting
+    # but method arg is typed this way for backwards compatibility
+    if isinstance(useKeySignature, key.KeySignature):
+        ksLast = useKeySignature
+    else:
+        ksLast = None
+
+    for i, m in enumerate(s):
+        # if beyond the first measure, use the pitches from the last
+        # measure for context (cautionary accidentals)
+        # unless this measure has a key signature object
+        if i > 0:
+            pitchPastMeasure = None
+            if m.keySignature is None:
+                pitchPastMeasure = s[i - 1].pitches
+            elif ksLast:
+                # If there is any key signature object to the left,
+                # just get the chromatic pitches from previous measure
+                # G-naturals in C major following G-flats in F major need cautionary
+                # G-naturals in C major following G-flats in Db major don't
+                ksLastDiatonic = [p.name for p in ksLast.getScale().pitches]
+                pitchPastMeasure = [p for p in s[i - 1].pitches
+                    if p.name not in ksLastDiatonic]
+            # Get tiePitchSet from previous measure
+            try:
+                previousNoteOrChord = s[i - 1][note.NotRest][-1]
+                tiePitchSet = getTiePitchSet(previousNoteOrChord)
+                if tiePitchSet is not None and m.keySignature is not None:
+                    # Get the diatonic pitches in this (new) key
+                    # and limit tiePitchSet to just those
+                    # Disregard tie continuation on pitches foreign to new key
+                    ksNewDiatonic = [p.name for p in m.keySignature.getScale().pitches]
+                    tiePitchSet = {tp for tp in tiePitchSet if tp in ksNewDiatonic}
+            except (IndexError, StreamException):
+                pass
+
+        if m.keySignature is not None:
+            ksLast = m.keySignature
+
+        m.makeAccidentals(
+            pitchPast=pitchPast,
+            pitchPastMeasure=pitchPastMeasure,
+            useKeySignature=ksLast,
+            alteredPitches=alteredPitches,
+            searchKeySignatureByContext=False,
+            cautionaryPitchClass=cautionaryPitchClass,
+            cautionaryAll=cautionaryAll,
+            inPlace=True,
+            overrideStatus=overrideStatus,
+            cautionaryNotImmediateRepeat=cautionaryNotImmediateRepeat,
+            tiePitchSet=tiePitchSet,
+        )
 
 def iterateBeamGroups(
     s: 'music21.stream.Stream',
@@ -1882,6 +1972,13 @@ class Test(unittest.TestCase):
         self.assertEqual(str(cm.exception),
             'meterStream is neither a Stream nor a TimeSignature!')
 
+    def testMakeAccidentalsInMeasureStreamException(self):
+        from music21 import converter
+        p = converter.parse(self.allaBreveBeamTest)
+        with self.assertRaises(ValueError) as cm:
+            makeAccidentalsInMeasureStream(p.measure(1))
+        self.assertIn('must contain only Measures', str(cm.exception))
+
 
 # -----------------------------------------------------------------------------
 


Test patch:
diff --git a/music21/braille/test.py b/music21/braille/test.py
index 1231add16..22e655f5e 100644
--- a/music21/braille/test.py
+++ b/music21/braille/test.py
@@ -1880,8 +1880,8 @@ class Test(unittest.TestCase):
             'tinynotation: 12/8 e2.~ e8 f# g# a b- gn c d e f4.~ f8 e f g f e f2.').flat
         bm.insert(0, key.KeySignature(-1))
         bm.makeNotation(inPlace=True, cautionaryNotImmediateRepeat=False)
-        # m = bm.getElementsByClass('Measure')
-        # m[1].notesAndRests[3].pitch.accidental.displayStatus = False
+        m = bm.getElementsByClass('Measure')
+        m[1].notesAndRests[3].pitch.accidental.displayStatus = False
         self.s = bm
         self.b = '''
         ⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠣⠼⠁⠃⠦⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀
@@ -3174,7 +3174,7 @@ Barline final ⠣⠅
         bm.insert(0, key.KeySignature(3))
         bm.makeNotation(inPlace=True, cautionaryNotImmediateRepeat=False)
         bm.getElementsByClass('Measure').first().padAsAnacrusis(useInitialRests=True)
-        # bm.getElementsByClass('Measure')[3][1].pitch.accidental.displayStatus = False
+        bm.getElementsByClass('Measure')[3][1].pitch.accidental.displayStatus = False
         # remove cautionary accidental display
         for m in bm:
             m.number -= 1
diff --git a/music21/stream/tests.py b/music21/stream/tests.py
index dd95f2efd..915215853 100644
--- a/music21/stream/tests.py
+++ b/music21/stream/tests.py
@@ -2601,6 +2601,48 @@ class Test(unittest.TestCase):
                              '%s failed, %s != %s' %
                                 (i, allNotes[i].pitch.accidental.displayStatus, ds[i]))
 
+    def testMakeNotationTiesKeyless(self):
+        from music21 import converter
+        p = converter.parse('tinynotation: 4/4 f#1~ f#1')
+        # Key of no sharps/flats
+        p.measure(1).insert(0, key.KeySignature(sharps=0))
+        # calls makeAccidentalsInMeasureStream()
+        p.makeNotation(inPlace=True)
+        self.assertIs(p.measure(2).notes.first().pitch.accidental.displayStatus, False)
+
+    def testMakeNotationTiesKeyChange(self):
+        from music21 import converter
+
+        p = converter.parse('tinynotation: 4/4 f#1~ f#1')
+        # Insert key change where held-over note is diatonic
+        p.measure(2).insert(0, key.KeySignature(sharps=1))
+        made = p.makeNotation()
+        self.assertIs(made.measure(2).notes.first().pitch.accidental.displayStatus, False)
+
+        p = converter.parse('tinynotation: 4/4 f#1~ f#1')
+        # Insert key change where held-over note is chromatic
+        p.measure(2).insert(0, key.KeySignature(sharps=-1))
+        made = p.makeNotation()
+        self.assertIs(made.measure(2).notes.first().pitch.accidental.displayStatus, True)
+
+        p = converter.parse('tinynotation: 4/4 b1~ b1')
+        # Same, but with a natural
+        p.measure(2).insert(0, key.KeySignature(sharps=-1))
+        made = p.makeNotation()
+        self.assertIs(made.measure(2).notes.first().pitch.accidental.displayStatus, True)
+
+        p = converter.parse('tinynotation: 4/4 f#1~ f#1')
+        p.measure(1).insert(0, key.KeySignature(sharps=-1))
+        # This is no longer a key "change", should still work based on the tie
+        made = p.makeNotation()
+        self.assertIs(made.measure(2).notes.first().pitch.accidental.displayStatus, False)
+
+        # Wipe out the tie; accidental should be reiterated
+        for n in p.flat.notes:
+            n.tie = None
+        made_no_ties = p.makeNotation()
+        self.assertIs(made_no_ties.measure(2).notes.first().pitch.accidental.displayStatus, True)
+
     def testMakeAccidentalsOctaveKS(self):
         s = Stream()
         k = key.KeySignature(-3)