Add makeNotation=False option to musicxml export and emit warnings on malformed streams

[jacobtylerwalls]

Previously
Showing or writing a stream always packed it in a score (if needed) and then made a deep copy and ran makeNotation() and makeRests() on that copy. (This was partially avoidable only by calling ScoreExporter directly, such as the testing tool musicxml.m21ToXml.Test.getET() does.)

Now

• show() and write() get a makeNotation argument that when set False, avoids making a deepcopy and making notation and making rests. Some deepcopies are still made during the packing of non-Scores into Scores, but makeNotation and makeRests calls (or their substitutes fixupNotationMeasured, etc.) are still avoided on the top level Score once packing is done.
• Fixes Warnings and documentation about expected stream nesting #870 : Emit a warning when streams fail isWellFormedNotation() on show or write

Refs #763

Future

We've discussed moving the static methods packing m21 objects into a Score to a more general location, such asbase.Music21Object.pack() with overridden methods on Stream, GeneralNote, etc. I've started a bit of that, but I'm inclined to make it a separate PR.

[coveralls]

Coverage increased (+0.02%) to 92.304% when pulling f6a5a44 on jacobtylerwalls:no-copy-on-write into e030bee on cuthbertLab:master.

[gregchapman-dev]

Can we get PR 996 merged into master soon, please? I’ve been using it for over a month with great success.

I’d like to move to latest, to pick up other fixes (and be a good tester for other changes).

[jacobtylerwalls]

Thanks to Michael for so many reviews last night. Thanks, Greg, for testing this one out, too.

Just to follow up on a conversation you and I had offline, Greg, since that time I've discovered a handy way to install from a local (i.e. git-tracked) directory into your pip-managed environment with pip3 install -e so that you can also be working from another project directory:

So let's say you needed to rebase this most excellent and salutary PR on top of latest like this:

cd music21
git checkout master
git pull upstream master
gh pr checkout 996
git rebase -i master
pip3 install -e music21
cd ../whatever-other-package-you-need-to-work-from

And it shows that pip will use my local install:

$ pip3 list
music21                            7.0.6a1             /Users/jwalls/music21

I just make sure I never EVER merge anything into my own local master branch so that I can always chuck everything local and pull cleanly from upstream.

[mscuthbert]

Looks good -- I like the concept. However, I don't like adding a boolean to lots of signatures (maybe more later as we find them) for an advanced feature. All of the Exporters are classes, so it would be better to set an attribute in the class after it's been set once. (for sub exporters, they can even look to .parent.makeNotation instead).

Thanks!

[mscuthbert]

I'd like eventually to document in the **keywords the ones that can be passed in.

[jacobtylerwalls]

Yep, we'd need a matrix for show and write on one axis and formats on the other.

[mscuthbert]

Unclear to me why we should be searching for Score in classes -- if someone is using makeNotation=False they should know what they're doing and give a well-formed object. This allows reverting the .parse(makeNotation=False) notes

[jacobtylerwalls]

So, in that case, if someone does myPart.show(makeNotation=False), should we just raise MusicXMLExportException?

[mscuthbert]

I'm trying to understand what the motivation for this attribute is except "I'm a super power user working with 7,000 scores and I want to speed something up" in which case I hope they'll realize that the user's guide is there to help.

I was going to ask in fact that the documentation for "makeNotation=False" says that it is a very advanced feature that assumes you know the ins and outs of music21 and musicxml fully before running it. Just trying to save support time in the future. (It's why I immediately close issues for "I wanted to use _coreInsert() because it should be faster, but I couldn't get it to work, can you help?")

[jacobtylerwalls]

Ya, happy to add a caveat. I think the motivation was that makeNotation fiddles with beams, Tuplet brackets, accidentals etc., and so you can't necessarily depend on changes you made to the Python streams making it out the door. At least that's how I ended up here with tuplets long ago.

[jacobtylerwalls]

That said, those hiccups may have been minimized after #1005 which included some of what was in an earlier version of this PR -- reading from actual streamStatus instead of streamStatus of a dummy stream never exposed to the user. But, we've also talked about adding things to makeNotation/export so still good to have, I think.

[jacobtylerwalls]

Great, I agree that an instance attribute is better. Diff is smaller now.

[mscuthbert]

LGTM!

For really uncommon attributes I usually don't even put a setter in __init__ -- just let people set the attribute on the object itself (ex = ScoreExporter(sc); ex.makeNotation = False), again just to not draw too much attention to it. Python, not being Java, does not need getters and setters for everything.

[jacobtylerwalls]

For really uncommon attributes I usually don't even put a setter in init -- just let people set the attribute on the object itself (ex = ScoreExporter(sc); ex.makeNotation = False), again just to not draw too much attention to it. Python, not being Java, does not need getters and setters for everything.

total sense = made. just doing as advised though :-) #996 (comment)

[mscuthbert]

total sense = made. just doing as advised though :-)

Ah, I meant setting self.makeNotation = True in the constructor __init__ function body, not putting it in the constructor arguments. How confusing that Python and other languages uses the same term to mean two different things.

[jacobtylerwalls]

Ah, it was I who made the leap to the signature, then, got it.