Interpret empty <forward> tags as rests on Finale documents only (derived from PR #1636, thanks @TimFelixBeyer!) #1777
Conversation
…d['software'] as it was being constructed, instead of afterward, because Metadata() actually adds a software item for the music21 version that read the file, and I don't want to be confused by that.
|
@jacobtylerwalls What do you think? |
|
FWIW I don't think this is enough to fully fix the issue. A lot of additional changes in my PR are centered around avoiding unnecessary calls to |
|
Yeah, I just realized that. Trying again. |
|
(and sorry about jumping all over this, but I just remembered that my 2 nested PRs depend on this, and I need to get it done ASAP; remind me NEVER to have PRs that depend on each other again, this sucks a lot.) |
|
While I personally would love this change, when I proposed this version in my PR (#1636 (comment)) it wasn't accepted (explanation here: #1636 (comment)) I know it's a lot, but I highly encourage you to read through the full thread in #1636 to understand the maintainers' perspective. |
|
Yeah, this is my attempt to get what you and I both need, and also make Jacob happy. (Hi @jacobtylerwalls!) |
|
Hoping we can ignore the slight decrease in coverage. I promise my next two PRs will increase coverage in MusicXML read/write a lot. |
|
OK, @jacobtylerwalls, I think this meets everyone's needs. This is the first of three PRs of mine that need to go in sequentially, the next one is PR #1768 (fixes many Spanner and SpannerAnchor-related bugs) and the last one is PR #1766, a new (better) implementation of PedalMarks that replaces the one that was merged onto master recently. My goal is to get that merged onto master before @mscuthbert makes the next music21 release (otherwise I think we might have to remove the PedalMark support that's on master). Can we expedite getting this one in, since it is the first of the blockers for that PedalMark work? |
Hi -- this is some very complex and subtle code changes whose discussions etc. are now spread over 4 PRs, and it's getting more than a bit frustrating to me. I'm not going to review any more code until I have a clear prose explanation for how the proposed solution to the problem given here differs from and is superior to the previous proposed PRs/solutions and, importantly, why it is significantly superior to the status quo. A superior solution, btw., does not add epicycles within epicycles of added complexity for all parts of the system to improve one thing. I haven't pushed enough against the complexity in terms of future dev's time and energy of some proposed solutions. Thanks! |
|
@jacobtylerwalls I think you should speak up here. I think this is what you were asking for, but perhaps I have missed something. |
|
And for the record, Myke, what's happening is that as I work on pedal marks, I am finding bugs in other parts of the system that block me (granted, some of them are my bugs, but I'm not adding layers of complexity, I'm fixing long-standing bugs). |
|
I will work on some prose about these three related PRs, though. You make a good point that I haven't described this for you very well at all. |
|
A description of the three related PRs (this is one of them) has been added to PR #1766. |
|
Taking a look |
jacobtylerwalls
left a comment
jacobtylerwalls
left a comment
There was a problem hiding this comment.
Thanks, as agreed in #1636 this is an improvement in correctness (limit workaround to Finale only) and net simplicity (given fewer makeRests() calls mixed in with parsing).
music21/musicxml/xmlToM21.py
Outdated
| self.parts = [] | ||
|
|
||
| self.musicXmlVersion = defaults.musicxmlVersion | ||
| self.wasWrittenByFinale = False |
There was a problem hiding this comment.
I think it would be better to localize this in xmlForward() along the lines I suggested here, checking metadataObj.software. A helper function could be extracted to do this checking.
I see from your commit message you tried this and found that setEncoding() is lossy -- we lose information about whether music21 was already in the software tag on the document -- but I think that shouldn't stop us from doing the right thing; we should just have at most one parser-level variable about that rather than one parser-level variable (and lower-order variables) about Finale-ness.
Checking in xmlForward should be fast, since we can depend on the metadata object always existing at the the top of the stream (no long O(n) searches finding no metadatas anywhere):
music21/music21/musicxml/xmlToM21.py
Lines 822 to 823 in 52fc784
You'd have to change that coreInsert (as modeled in that diff I linked).
@mscuthbert do you agree that would achieve better "locality of behavior" than adding more parser attributes for edge cases?
There was a problem hiding this comment.
Assuming I'm understanding this correctly, I'm not sure much simplicity is gained by replacing self.wasWrittenByFinale with self.music21WasActuallyInTheDocsSoftwareMetadata (or some other better name). Maybe a rename with the existing semantics? self.applyFinaleWorkarounds, or something like that, that feels more like it is a legit parser-level variable.
There was a problem hiding this comment.
I'm also going to argue that performing the "loop over the software metadata" for every <forward> element is going to be significantly more expensive than doing it once per document.
There was a problem hiding this comment.
Yep, do it once. I believe that the order of software tags written out by music21 is the order we found was most common at the time (17 years ago) for how to layer them. So we really only need to look if the top layer is Finale.
There was a problem hiding this comment.
self.applyFinaleWorkarounds sounds very good. Or if we want to make it very generic self.softwareWorkarounds: set[str] = {'finale'}
|
Also, please update the PR title to something that will be more helpful years from now. |
gregchapman-dev
changed the title
|
All done except for the continuing discussion of "locality of behavior" regarding |
jacobtylerwalls
left a comment
jacobtylerwalls
left a comment
There was a problem hiding this comment.
Thanks! I'll wait a few days to see if @mscuthbert wants to weigh in.
|
Huge thanks to @gregchapman-dev and to @jacobtylerwalls and to @TimFelixBeyer -- I finally get what through all the weeds and difficulties what we're essentially all in agreement we're trying to do. And based on finally understanding everything, my normal worry-wart objection "what if the next version of Finale fixes this?" is now moot by experience. Let me do a last readthrough and see what to do. |
music21/musicxml/xmlToM21.py
Outdated
| r.style.hideObjectOnPrint = True | ||
| self.addToStaffReference(mxObj, r) | ||
| self.insertInMeasureOrVoice(mxObj, r) | ||
| if self.parent.parent.wasWrittenByFinale: |
There was a problem hiding this comment.
One of the few times I wish Python were Javascript -- need to check for grandparent existence...
if (self.parent
and self.parent.parent
and self.parent.parent.wasWrittenbyFinale
):
Vote YES on PEP 505
Add safety for MeasureParsers created w/o Part/ScoreParsers (testing, extensions, etc.) Add TESTS to make sure that Finale workarounds are only done if the score was last exported by Finale. Tests are mostly on processing Encoding, which suddenly matters a lot! :-)
|
Thanks everyone -- I realize now that the details prevented me from seeing the big picture. In my last push I renamed some things to help me understand what things (like storing the final rest made from I added a test of the encoding parser (long overdue before this even) and now I think I fully understand how this PR improves the system without making it too much more complex. Approved and merging. Please take a minute after this to close issues/PRs that are now fixed -- or to reformulate them (renaming helps a LOT!) so that I can understand their relevance, and let's get some more Piano marks etc. in. Greg -- seeing on the list that m21 isn't currently working on Python 3.13 for Windows users, I might make a quick 9.7 release ASAP to help them even if PianoMarks aren't in fast. But I'll do 9.9 quickly afterwards to get that in, promise. (In case eyebrows go up -- since m21 9 worked with the latest Python w/ numpy 1.26 and now less than a year later no longer works with the latest Python w/ recent numpy, I'm considering it more of a bug-fix release than a non-backwards-compatible release, since our dependencies didn't think about 18-24 month support on their end). |
|
Closing #1636 (Harvard PR) |
5 participants
Note that I had to check md['software'] as it was being constructed, instead of afterward, because Metadata() actually adds a software item for the music21 version that read the file, and I don't want to be confused by that.