Implement new spanner.SpannerAnchor class

[gregchapman-dev]

Fixes #1402

[gregchapman-dev]

An attempt at fixing issue #1402.

[coveralls]

Coverage: 93.05% (-0.002%) from 93.052% when pulling 3be7d72 on gregchapman-dev:gregc/spannerAnchor into aa7b068 on cuthbertLab:master.

[mscuthbert]

@gregchapman-dev -- it seems like your PRs always coincide with a new release of mypy. As far as I can see none of the current errors are from your work. I just finished some work at WUSTL and visiting family over the weekend. Hope to get a patch for mypy in by tuesday and give this a review by then.

It'd be really helpful though if the PR could include an example of how to use SpannerAnchor and an example of where it clarifies a situation (like with a Crescendo, etc.) -- I think that people will want to see this. And a test in the musicxml output to show that it's working.

[gregchapman-dev]

Yes, mypy is very unhappy. Hilariously, I had just updated mypy locally, so I thought it was only my problem, not music21's. Ah well...

Yes, I will be adding examples and tests. Still getting it working. I'm attempting to use/support SpannerAnchors in my Humdrum parser, Humdrum writer, MEI parser, and in musicdiff, so I am pounding the edges of this pretty hard. So far so good.

[jacobtylerwalls]

Apparently the INFO messages from mypy won't cause CI to fail. Just addressing the ERRORs should be enough 🙏🏻

[gregchapman-dev]

Ah, shoot. Well, that's what happens when you write a test: you find a big bug. I will be making a more significant change to m21ToXml.parseFlatElements... All my usage so far had been putting any SpannerAnchors up in the Measure (not in the Voice with other Notes), which worked fine, BUT... SpannerAnchors together in a stream with Notes don't export with correct offsets because SpannerAnchors can be positioned in the middle of a notes (and that note, along with its entire duration, has already pushed the offset beyond where the SpannerAnchor should land). I will be doing a quick SpannerAnchor-only pass in parseFlatElements, then do a backup and then export the remaining objects as before. The resulting MusicXML will look as if all the SpannerAnchors were in their own voice (without requiring that clients carefully do that when constructing their music21 score).

[gregchapman-dev]

Hmmm.... that would work but spanners that mix notes and spannerAnchors get confused by this approach.

        score = stream.Score()
        part = stream.Part()
        score.insert(0, part)
        measure = stream.Measure()
        part.insert(0, measure)
        voice = stream.Voice()
        measure.insert(0, voice)

        n = note.Note('C', quarterLength = 4)
        sa1 = spanner.SpannerAnchor()
        sa2 = spanner.SpannerAnchor()
        voice.insert(0, n)
        voice.insert(2, sa1)
        voice.insert(4, sa2)
        cresc = dynamics.Crescendo(n, sa1)  # cresc from n to sa1
        dim = dynamics.Diminuendo(sa1, sa2)   # dim from sa1 to sa2
        score.append((cresc, dim))

gives me:

    <!--========================= Measure 0 ==========================-->
    <measure number="0">
      <attributes>
        <divisions>10080</divisions>
      </attributes>
      <forward>
        <duration>20160</duration>
      </forward>
      <direction placement="below">
        <direction-type>
          <wedge number="2" spread="15" type="diminuendo" />
        </direction-type>
      </direction>
      <direction placement="below">
        <direction-type>
          <wedge number="1" spread="15" type="stop" />
        </direction-type>
      </direction>
      <forward>
        <duration>20160</duration>
      </forward>
      <direction placement="below">
        <direction-type>
          <wedge number="2" spread="0" type="stop" />
        </direction-type>
      </direction>
      <backup>
        <duration>40320</duration>
      </backup>
      <direction placement="below">
        <direction-type>
          <wedge number="1" spread="0" type="crescendo" />
        </direction-type>
      </direction>
      <note>
        <pitch>
          <step>C</step>
          <octave>4</octave>
        </pitch>
        <duration>40320</duration>
        <type>whole</type>
      </note>
    </measure>

I think all the wedge starts and stops are at the correct measure offset, but.... they are out of order pretty badly, and Musescore and Finale both get confused (differently) by this. Crap. Can we require that if you use a SpannerAnchor you must use one for both ends of the spanner? (I'm guessing no.)

[gregchapman-dev]

OK, I figured out a way to get the wedge starts and stops to be (pretty much) in document order when there are mixed GeneralNotes and SpannerAnchors being used in spanners. I carefully don't do it unless there are SpannerAnchors in the flat voice/measure, since it would change the emitted MusicXML quite a bit in cases where it didn't need to.

Let me know what you think; I suspect there is much I haven't thought of here.

[mscuthbert]

closing and reopening to use a pinned version of mypy.

[mscuthbert]

Ignore the tests beginning with "PyLint / " -- they're the old system. If this PR is updated from master, they'll go away.

[jacobtylerwalls]

Thanks for letting me find some time to review. I left some minor feedback.

More broadly, I'm wondering if there's a smaller solve here. Some of the issues I mentioned about repetitious code might be obviated with something like this. As I understand it, you just want "two passes" to emit all the spanner anchors first. So what about...

        objIteratorSpannerAnchors: OffsetIterator[base.Music21Object] = OffsetIterator(
            m).addFilter(lambda el: _hasPrePostObjectSpanners(el))
        objIteratorAllElse: OffsetIterator[base.Music21Object] = OffsetIterator(
            m).addFilter(lambda el: not _hasPrePostObjectSpanners(el))
        for objIterator in (objIteratorSpannerAnchors, objIteratorAllElse)
            for objGroup in objIterator:
                # ... existing code

• You might need to manage backupAfterward somehow in between the two passes, but that should be enough to get you started.
• I'm not sure I have the filter right, but if you let me know where it's wrong, that would help me understand the PR better.

WDYT? 🤔

[gregchapman-dev]

I've fixed the simple requests here. The DRY stuff and reordering to avoid weirdly incremented voice numbers are still being worked on.

[gregchapman-dev]

All feedback has been incorporated now, except for the DRY stuff, which I will now take a shot at.

[gregchapman-dev]

I have factored out moveForward and moveBackward. I did not do the thing where I loop over two iterators running the same code for each, since what happens when running each offsetIterator is quite different. I've put in a block comment describing the two passes, for your enjoyment.

[gregchapman-dev]

I believe I am done responding to @jacobtylerwalls' review now. Time for another review pass? I'm not happy that coverage decreased by 0.002%, but I hope that's OK; I've tested the new code pretty thoroughly (and coverage isn't showing me anything that is new, and not covered... not sure where the 0.002% is coming from).

[gregchapman-dev]

Heh. I cheated and deleted two unnecessary lines of code to get better coverage.

[jacobtylerwalls]

Thanks for the changes, they help me digest what's going on more fully!

I did not do the thing where I loop over two iterators running the same code for each, since what happens when running each offsetIterator is quite different. I've put in a block comment describing the two passes, for your enjoyment.

Fair enough, was just an idea. Now that this PR's diff is smaller, it looks good. I took two minutes and found that my idea doesn't end up much saving much net code, and your version is more explicit.

my attempt

diff --git a/music21/musicxml/m21ToXml.py b/music21/musicxml/m21ToXml.py
index d71a4479e..3f6d34e42 100644
--- a/music21/musicxml/m21ToXml.py
+++ b/music21/musicxml/m21ToXml.py
@@ -3288,56 +3288,65 @@ class MeasureExporter(XMLExporterBase):
         # beginning and does all the prePostObjectSpanners processing.
         # If there are no SpannerAnchors, we do one pass, doing any prePostObjectSpanners
         # processing as we process each element.
+        objIteratorSpannerAnchors: OffsetIterator[base.Music21Object] = OffsetIterator(
+            m).addFilter(lambda el: self._hasPrePostObjectSpanners(el))
+        objIteratorAllElse: OffsetIterator[base.Music21Object] = OffsetIterator(
+            m).addFilter(lambda el: not self._hasPrePostObjectSpanners(el))
+        for passNumber, objIterator in enumerate(objIteratorSpannerAnchors, objIteratorAllElse):
+            firstPass = passNumber == 0
+            for objGroup in objIterator:
+                hasSpannerAnchors: bool = bool(m[spanner.SpannerAnchor])
+
+                # First (and possibly only) pass:
+                # Group all objects by offsets and then do a different order than normal sort.
+                # That way chord symbols and other 0-width objects appear before notes as much as
+                # possible.
+
+                # Second pass: If this flat stream (measure/voice) contains any SpannerAnchors, we
+                # will perform a second pass, emitting any pre/postList for any object that is
+                # functioning as a spanner anchor (the SpannerAnchors, as well as any GeneralNotes
+                # that start or end spanners).
+                objGroup: list[base.Music21Object]
+                objIterator: OffsetIterator[base.Music21Object] = OffsetIterator(m)
+                for objGroup in objIterator:
+                    groupOffset = m.elementOffset(objGroup[0])
+                    offsetToMoveForward = groupOffset - self.offsetInMeasure
+                    if offsetToMoveForward > 0:
+                        if not firstPass or any(
+                            isinstance(obj, (note.GeneralNote, clef.Clef)) for obj in objGroup):
+                            # gap in stream between GeneralNote/Clef objects: create <forward>
+                            self.moveForward(offsetToMoveForward)
+
+                    notesForLater = []
+                    for obj in objGroup:
+                        # we do all non-note elements (including ChordSymbols not written as chord)
+                        # first before note elements, in musicxml
+                        if firstPass and isinstance(obj, spanner.SpannerAnchor):
+                            # save for second pass
+                            continue
 
-        hasSpannerAnchors: bool = bool(m[spanner.SpannerAnchor])
-
-        # First (and possibly only) pass:
-        # Group all objects by offsets and then do a different order than normal sort.
-        # That way chord symbols and other 0-width objects appear before notes as much as
-        # possible.
-        objGroup: list[base.Music21Object]
-        objIterator: OffsetIterator[base.Music21Object] = OffsetIterator(m)
-        for objGroup in objIterator:
-            groupOffset = m.elementOffset(objGroup[0])
-            offsetToMoveForward = groupOffset - self.offsetInMeasure
-            if offsetToMoveForward > 0 and any(
-                    isinstance(obj, (note.GeneralNote, clef.Clef)) for obj in objGroup):
-                # gap in stream between GeneralNote/Clef objects: create <forward>
-                self.moveForward(offsetToMoveForward)
-
-            notesForLater = []
-            for obj in objGroup:
-                # we do all non-note elements (including ChordSymbols not written as chord)
-                # first before note elements, in musicxml
-                if isinstance(obj, spanner.SpannerAnchor):
-                    # nothing to do with this (only prePostObjectSpanners, and
-                    # we will do that below)
-                    continue
-
-                if isinstance(obj, note.GeneralNote) and (
-                    not (isHarm := isinstance(obj, harmony.Harmony))
-                    or (isHarm and obj.writeAsChord)
-                ):
-                    notesForLater.append(obj)
-                else:
-                    self.parseOneElement(obj, skipPrePostObjectSpanners=hasSpannerAnchors)
-
-            for n in notesForLater:
-                if n.isRest and n.style.hideObjectOnPrint and n.duration.type == 'inexpressible':
-                    # Prefer a gap in stream, to be filled with a <forward> tag by
-                    # fill_gap_with_forward_tag() rather than raising exceptions
-                    continue
-                self.parseOneElement(n, skipPrePostObjectSpanners=hasSpannerAnchors)
-
-        # Second pass: If this flat stream (measure/voice) contains any SpannerAnchors, we
-        # will perform a second pass, emitting any pre/postList for any object that is
-        # functioning as a spanner anchor (the SpannerAnchors, as well as any GeneralNotes
-        # that start or end spanners).
+                        if isinstance(obj, note.GeneralNote) and (
+                            not (isHarm := isinstance(obj, harmony.Harmony))
+                            or (isHarm and obj.writeAsChord)
+                        ):
+                            notesForLater.append(obj)
+                        else:
+                            self.parseOneElement(obj, skipPrePostObjectSpanners=firstPass and hasSpannerAnchors)
 
-        if hasSpannerAnchors:
-            # return to the beginning of the measure, to emit any SpannerAnchors.
-            if self.offsetInMeasure:
-                self.moveBackward(self.offsetInMeasure)
+                    for n in notesForLater:
+                        if n.isRest and n.style.hideObjectOnPrint and n.duration.type == 'inexpressible':
+                            # Prefer a gap in stream, to be filled with a <forward> tag by
+                            # fill_gap_with_forward_tag() rather than raising exceptions
+                            continue
+                        self.parseOneElement(
+                            n,
+                            skipPrePostObjectSpanners=firstPass and hasSpannerAnchors,
+                            prePostObjectSpannersOnly=not firstPass,
+                        )
+
+                if firstPass and hasSpannerAnchors and self.offsetInMeasure:
+                    # return to the beginning of the measure, to emit any SpannerAnchors.
+                    self.moveBackward(self.offsetInMeasure)
 
             objIterator = OffsetIterator(m)
             for objGroup in objIterator:

[gregchapman-dev]

If we can get this merged into master soon, I'll merge master back into gregc/ottavaTransposeFix so that PR #1486 will become clearer.

[mscuthbert]

Hi @gregchapman-dev -- comments related to this PR were put in the Ottava PR; they're pretty light and I think can be put in here to merge this before that PR is completely ready.

[gregchapman-dev]

OK, all the review comments in the Ottava PR that had to do with SpannerAnchors have been responded to here as well.

[mscuthbert]

It looks amazing! THANK YOU. I fixed one little music21 style thing (I might drop the spaces required around operators in cases like this eventually, but if we do I'll do it everywhere).

This is spectacular. So happy to merge.

[mscuthbert]

This is the most AWESOME implementation of a refactor ever. I'm so happy with how the logic flows here now. Thank you thank you thank you thank you!

[mscuthbert]

Will merge after the button is active for me. Then back to ottava!