Ottava transposition fixes

[gregchapman-dev]

No description provided.

[gregchapman-dev]

This is just the first few things mentioned in issue #1419. Lots more work to do, but at this point the notes that are actually in the Ottava are transposed correctly. I'm now off to make sure all the notes are in the Ottava (and in other Spanners as well).

[gregchapman-dev]

Oh, you might notice that I have the SpannerAnchor implementation from PR #1479 included here. I will continue that work in this PR as well (I think it's done, but I might find some bugs as I work on Ottavas).

[mscuthbert]

Oh, you might notice that I have the SpannerAnchor implementation from PR #1479 included here. I will continue that work in this PR as well (I think it's done, but I might find some bugs as I work on Ottavas).

It'd be better if we can get that one merged first so it's easier to see the separation of the two. I'll look at it now.

[mscuthbert]

Or.......(thinking out loud too later) .... what happens if instead of using the spanner anchor, we check for non-zero Spanner duration in the case of one element-spanners?

[gregchapman-dev]

I actually tried that first (on a different branch), but a Spanner with a duration can't reach into subsequent Measures. You gotta have an anchor (GeneralNote or SpannerAnchor) over in that Measure.

[gregchapman-dev]

Interesting test failure: Since I'm no longer checking s.atSoundingPitch is True (in m21ToXml.py's ScoreExporter.parse() and PartExporter.parse()) before calling s.toWrittenPitch (because the ottava transposition code in toWrittenPitch needs to run in all cases), _treatAtSoundingPitch is now raising an exception because s.atSoundingPitch is 'unknown' in all contextSites. This is the case in many tests. @mscuthbert and @jacobtylerwalls, I'm wondering if _treatAtSoundingPitch should interpret s.atSoundingPitch == 'unknown' as "at written pitch" instead of raising an exception, since that's what m21ToXml.py was effectively doing. I will do that for now, but please let me know if there's something else I should do here.

[gregchapman-dev]

Ah, even better, just return 'unknown' from _treatAsSoundingPitch instead of raising an exception, skipping the whole transposition side effect.

[coveralls]

Coverage: 93.068% (+0.01%) from 93.058% when pulling 498a2a9 on gregchapman-dev:gregc/ottavaTransposeFix into f04ace2 on cuthbertLab:master.

[gregchapman-dev]

Huh. I think I worked around the circular imports (but I had to leave the searchStream parameter un-type-hinted). Hopefully there's a better way.

[gregchapman-dev]

@jacobtylerwalls, please take a look at my changes to xmlToM21.py, which fill out any Ottavas during MusicXML parse. I got stymied a bit by separateOutPartStaves, since I need to search the correct partStaff for the intermediate elements. I ended up finding the correct partStaff using a potentially very expensive search for ottava.getFirst() in each partStaff. This is crude, but it works.

Better, I think, would be to modify separateOutPartStaves to (1) put the Ottava in the correct partStaff (it currently puts all spanners in the first partStaff, which is fine for many spanners, but is definitely wrong for Ottavas), and then (2) fill out the Ottava right then while we have our hands on the right partStaff to search for intermediate elements. I'll take a shot at doing that, but any pointers would be helpful, since separateOutPartStaves is fairly complex.

[gregchapman-dev]

From the last checkin comment:

In xmlToM21.py, let's try filling in all spanners. Result: mozartTrioK581Excerpt
has two slurs whose startNote isn't in the searchStream. I'll figure that out shortly.

I figured it out. mozartTrioK581Excerpt has a slur that starts just before the end of the excerpt in both parts 1 (Clarinet) and 2 (violin 1). Our MusicXML parser is apparently seeing the next slur start in the document as a stop for that trailing slur. So we end up with two bad slurs: one that starts at the end of part1 and ends in measure 6 of part 2, and one that starts at the end of part 2 and ends in measure 6 of part 3 (violin 2). And since the Slur spanner ends up associated with the Part where it ends, we have a start note in the slur that is not in that Part. I no longer crash (yay) when this happens, but... I think there's a bug in the parser that gets tripped up by this not-that-well-formed-because-it's-an-excerpt MusicXML file. (If I remove the trailing slur starts, all is well.)

[gregchapman-dev]

I wrote up a new issue for the xmlToM21.py bug with slurs that start but don't stop (issue #1489).

[gregchapman-dev]

Hmmm... After a bunch of testing, I am slowly coming to the conclusion that while the utility "fillIntermediateSpannedElements" might be useful, I really only want to call it on Ottavas in xmlToM21.py:PartParser (and similarly in my own parsers). I keep running into problems with other spanners. For example, a Diminuendo that lasts exactly one quarter note (there's one quarter note in the spanner) gets stretched to a dotted-quarter because there's a dotted-quarter note at the same offset in a different Voice in the Measure. For an Ottava, that's appropriate, because that dotted-quarter falls under the Ottava as well, and may need to be transposed. But when a spanner is simply about duration, I think filling it is wrong. Thoughts, @jacobtylerwalls and @mscuthbert?

[gregchapman-dev]

Oh, filling an ArpeggioMarkSpanner should also be done (if the parser's file format doesn't already carefully describe all the chords/notes in the arpeggio, and instead only describes the top and bottom chords/notes). But that's a custom "vertical" fill algorithm that I need to work on now.

[gregchapman-dev]

Another commit with another big question for @mscuthbert and @jacobtylerwalls: Inheriting accidental display options during transposition is something that appears to be handled somewhat inconsistently, and I needed an option to force inheritance (1) at end of Humdrum parse, and (2) when comparing scores in musicdiff, where the visibility of an accidental in one score vs another is considered a difference worth noting (but of course, musicdiff can't compare values of notes in Ottavas until both versions of the Ottava have been transposed into a similar state).

So, in my most recent commit, I added an inheritAccidentalStatus: bool = False parameter to most transpose routines, so that I could request this behavior by passing inheritAccidentalStatus=True.

I think there is more work to do here, to make the behavior more consistent when the parameter is False, but that is riskier work than belongs here. Some routines don't inherit, some always do, and there's one that inherits all but Accidental.displayStatus.

[gregchapman-dev]

I think that will work! I've written the code changes for _transposeByInstrument (which is called by both toWrittenPitch and toSoundingPitch), and it's pretty simple. I'm going to finish that up (removing treatAsKeyChange everywhere) and push it for your approval.

[gregchapman-dev]

There ya go! I like this a lot better.

[gregchapman-dev]

I fixed the failing SieveScale test by making sure a Unison/Octave doesn't have fractional semitones. This only affected displayStatus, but I still don't know why that caused the test to fail. Whatever, I fixed a real bug that happened during that test, and the test stopped failing.

[gregchapman-dev]

Back to more tests.

[gregchapman-dev]

I think I've got this all done now. Test coverage is good, too.

[mscuthbert]

I think that will work! I've written the code changes for _transposeByInstrument (which is called by both toWrittenPitch and toSoundingPitch), and it's pretty simple. I'm going to finish that up (removing treatAsKeyChange everywhere) and push it for your approval.

I'm still not sure that this takes every potential place where displayStatus could be wrong after a diatonic transposition. I mention in the review transposition on an atonal score, but also transposition of a mid-measure instrument or key change. It seems like too much to trust when there's really only one place (makeAccidentals) which has been fire-tested against hiding an accidental that needs to be made.

I'm happy to have a keyword to toSoundingPitch() and toWrittenPitch() of preserveAccidentalDisplay: bool = False that gets passed to _transposeByInstrument() and if it's True then we run the routines.

Though this also looks like it should be a contextlib.contextmanager somewhere so it can be tested separately (see common.classTools.saveAttributes):

run_transpose = lambda: focus.transpose(trans, inPlace=True, classFilterList=classFilterList)
if preserveAccidentalDisplay and self.haveAccidentalsBeenMade():
     with saveAccidentalDisplayStatus(focus):
           run_transpose()
else:
     run_transpose()

Then it can be documented and tested separately and used elsewhere in the system or by others (your OMR programs?), and its usage in _transposeByInstrument becomes basically self-documenting.

Edit -- just because my mind is on it, might as well write the context manager code. Probably to go in stream.makeNotation?

import contextlib

@contextlib.contextmanager
def saveAccidentalDisplayStatus(s) -> t.Generator[None, None, None]:
     '''
     Restore accidental displayStatus on a Stream after an (inPlace) operation.  ...[etc. -- demo]
     '''
     displayStatuses: dict[int, bool | None] = {}
     for p in s.pitches:
           if p.accidental is not None:
                 displayStatuses[id(p)] = p.accidental.displayStatus
                 continue
           displayStatuses[id(p)] = False
      try:
          yield
      finally:
          for p in s.pitches:
                if p.accidental is not None:
                     p.accidental.displayStatus = displayStatuses.get(id(p), None)
                     continue
                if displayStatuses.get(id(p), False) is True:
                    p.accidental = pitch.Accidental(0)
                    p.accidental.displayStatus = True

with .get() added for safety (you never know what the inner routine might do...)

[mscuthbert]

Couple of small things regarding transpositionChangesPitchClass (probably not needed) and making preserveDisplayStatus optional in toSounding, etc. otherwise really close.

Thanks for the added tests.

[gregchapman-dev]

I'll have some time Friday to work through this latest review. Thanks!

[gregchapman-dev]

I believe I have responded to all review comments now. The only remaining issue is whether there is a better implementation of Interval.transpositionChangesPitchName, currently implemented as:

    @property
    def transpositionChangesPitchName(self) -> bool:
        if self.simpleName == 'P1' and float(self.semitones) == float(int(self.semitones)):
            return False
        return True

self.simpleName == 'P1' on its own does not return the correct True value for an Oblique Perfect Unison.

[gregchapman-dev]

pylint issues are not mine. Yet Another Lint-Gets-Pickier-Whenever-Greg-Pushes-A-Big-PR. 🙄

[mscuthbert]

Oh, geez -- this looks like it's pylint-dev/pylint#7494 -- I wish they wouldn't change something like this on minor releases which are supposed to be backwards compatible. Will look in morning. (I don;t think v9 will come out by first day of class. So we'll get this in before it is and then I'll have the students upgrade...)

[mscuthbert]

fixed -- closed and reopened to run again.

[mscuthbert]

Ah, I suppose transposing microtones can change the name of the pitch, but it doesn't have to:

In [20]: i = interval.Interval('P1')

In [21]: i.chromatic.semitones = 0.01

In [22]: p = pitch.Pitch('C4')

In [23]: for j in range(20):
    ...:     print(p.name)
    ...:     p.transpose(i, inPlace=True)
    ...: 
C
C
C
C
C
C
C
C
C
C
C
C
C
C
C
C

though there is something strange happening here:

In [24]: i = interval.Interval('P1')

In [25]: i.chromatic.semitones = 0.5

In [27]: p = pitch.Pitch('C4')

In [28]: for j in range(9):
    ...:     print(p.name, p.ps)
    ...:     p.transpose(i, inPlace=True)
    ...: 
C 60.0
C~ 60.5
C# 61.0
C# 61.0
C# 61.0
C# 61.0
C# 61.0
C# 61.0
C# 61.0

(why does it stop?

[gregchapman-dev]

This is getting deeper than I am happy addressing in this PR. Perhaps I should get rid of transpositionChangesPitchName, since it is so problematic, and simply perform that same check inline when I want to know if a transposition can safely maintain the existing accidental display status (i.e. if the transposition ends up leaving the note as it is, or transposing it by some number of (exact) octaves).

[gregchapman-dev]

Latest push does exactly that: inlines the check. I'm checking whether or not I can safely keep displayStatus. So any "truly perfect" unison or octave is fine.

[gregchapman-dev]

@mscuthbert Any chance of getting this merged soon? I'm happy to write up an issue about the microtone transposition issues, to work on separately.

[mscuthbert]

Thanks! All looks good -- now that we have pylint working again.