Support ornament accidentals #1545
Conversation
…t/Trill was not created with accid, and _never_ for HalfStep/WholeStep Mordent/Trill variations.
…h accidentals above, instead of one above and one below. (in anticipation of xmlToM21.py ornament accidentals work)
|
Implementation so far: Add accidental support to Turn, GeneralMordent, Trill. Parse from/write to MusicXML. Last step (still to do): makeAccidentals needs to add support for ornament accidentals. |
|
Yeah, I know, I have a bunch more tests to write. |
|
@mscuthbert @jacobtylerwalls Can I get some review of this implementation (while I work on makeAccidentals)? |
|
Hmmm... makeAccidentals needs to deal with Pitches, not just Accidentals. So... Maybe instead of ornament.accid/lowerAccid/upperAccid being Accidentals, I should just have them be strings ('flat', 'sharp', etc as in MusicXML). Then parsers and makeAccidentals can do ornament.resolveOtherPitches(srcPitch), which will actually set up an internal list of "other" pitches implied by the ornament (which will be returned by ornament.otherPitches, and included in stream.pitches). The accidentals on those pitches would mean exactly what they mean on note/chord pitches. And those accidentals might be set because of ornament.getSize(srcPitch) and current key, even if accid/upperaccid/lowerAccid is not set (but size is, or class name means something). And makeAccidentals can pay attention to them, and set displayStatus on them (parsers will set displayStatus on them too, of course). resolveOtherPitches(srcPitch) might need a different verb: realize, setup, create, compute, figureOut, make, or something like that. |
|
OK, initial implementation of everything is done. I need to write a bunch of tests (especially for makeAccidentals stuff), but this should be reviewable now. |
There was a problem hiding this comment.
Great work! Love it! Sorry that most individual comments are on the things I dislike, but it's a significant contribution.
Things requested to change are:
- please document interface before requesting review next time (I'd prefer to review an empty interface w/o implementation rather than an implementation without docs). It slows down the review immensely to have to guess what a routine does and how it handles corner cases.
- otherPitches doesn't have any meaning to me -- what is this?
- getSize -- issues with naming and the notion that the size returned will later be modified by key context etc. -- one routine is preferred that takes in all the information it needs to spit out the exactly right interval. Try to think of the use case where someone want to measure an ornament w/o realizing it.
- don't change the behavior of Stream.pitches.
- getters cannot raise exceptions. See note for reason.
- makeAccidentals is already way too long and complicated -- use helper functions to remove redundancy.
|
Thank you for the quick review (and sorry for the lack of documentation). I agree with nearly all of what you said, and the rest just raises questions, not disagreement. Hoping to find some time to work through most of this tomorrow. |
|
@mscuthbert I think I'm done here, ready for your re-review. Please do validate that my changes to pitch.py's test expectations are correct (that's the very last commit above). |
…tead of in the pitch.accidental = float setter (to minimize risk). Add Unpitched to the typing for all the realize-related methods.
…= float setter as out-of-scope and too risky.
|
I have removed the microtones-to-quarter-tones operation that I had added to pitch.accidental's float setter, and added it instead to the resolveOrnamentalPitches methods in Trill/Turn/GeneralMordent. Note that I had to work around a different bug in transpose where microtones get completely lost if you transpose in place (that I have written an issue for: #1553). I also addressed Jacob's review comments. |
…rotones), and fix it instead.
|
I've decided I'm not done yet. I've been playing with my Humdrum writer, making it write from these new Ornament objects, and I'm finding that sometimes the Ornament ends up with the wrong size, due to the fact that the ornamentalPitch was modified by an accidental on a note earlier in the measure (and no-one knows, so it ends up not modified). This is actually a bug in my Humdrum importer, where I know what the accidental should be, but I don't set accidentalName because I don't want to force it to be visible. But then getSize doesn't know about that accidental, so it gets the size wrong (and therefore the ornamentalPitch comes out wrong, too). So I think @mscuthbert's idea earlier (that ornament.accidentalName should actually be an Accidental object instead of a name string) is correct, so my parser can set the accidental, and then set that accidental's displayStatus to True if it is supposed to be visible. Then resolveOrnamentalPitches will need to copy that displayStatus to the resolved ornamentalPitch.accidental. (And all of this is doubled for Turns, where there are two ornamentalPitches, two accidentalNames, etc). |
…). Same with Turn.upperAccidentalName and lowerAccidentalName.
|
OK, I've made that change. I think I might be done here. |
|
Doing re-review now. Even before looking at the code, just want to say "Excellent work!" and thank you. |
There was a problem hiding this comment.
Great work! You'll be relieved to see that there are no conceptual changes requested, nothing about "wouldn't this be better if .accidental took a NeoRiemannianTransformation instead?" etc. :-)
There are some places where I'd like refactors for clarity (and making Stream.base not so darn big) and some docs that are missing, etc.
There are some significant typing improvements that can be done after this is merged.
Thanks!
|
The next review should be light enough that I'll get it on a mid-week review schedule when it comes in. Needed to wait for a holiday for this one. |
|
Thank you for the thorough review! I'll make these changes over the next few days. |
|
I believe I have addressed all @mscuthbert's latest review comments. |
music21/stream/base.py
Outdated
| self.makeOrnamentalAccidentals( | ||
| makeNotation.makeOrnamentalAccidentals( |
There was a problem hiding this comment.
Oh! it didn't even need self -- then definitely best not to live on Stream!
| from music21.stream import Stream | ||
| post = [] | ||
| for e in s.elements: | ||
| if isinstance(e, Stream): |
There was a problem hiding this comment.
this can be done without the import with if 'Stream' in e.classes which is why .classes is still there. NBD!
|
Thanks for taking it! |
Fixes issue #1507