Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Apply tuplet to multiple components to express 5/6 or 7/3 QL #763

Closed
wants to merge 36 commits into from

Conversation

jacobtylerwalls
Copy link
Member

@jacobtylerwalls jacobtylerwalls commented Jan 9, 2021

Fixed #572

Before: Durations such as 5/6 or 7/3 QL were expressed as non-power of 2 tuplets, which caused display problems.

Now: Upon extracting the largest type (such as 0.5 QL from 5/6 QL or 2.0 QL from 7/3 QL), if the remainder can be expressed as a single tuplet, express the largest type and the remainder in smaller components over which a tuplet can be applied.

Added support to the musicxml exporter for creating the necessary visual <tuplet> elements from each split component. Update: added a call to remake tuplet brackets inside splitAtDurations()

Update: added the keyword splitAtDurations to makeNotation() and moved the logic in the xml exporter. Update: calls new splitAtDurations method on Stream.

Update: adds attribute/slot expressionIsInferred

Update: extends tuplet brackets over tuplets that may not share the same type (3/2/eighth and 3/2/16th) but at least share the same ratio. This is to avoid unnecessarily proliferating start/stop brackets. Respected in MuseScore at least.

@coveralls
Copy link

coveralls commented Jan 9, 2021

Coverage Status

Coverage increased (+0.006%) to 92.088% when pulling 0c21c66 on jacobtylerwalls:components into b96cc99 on cuthbertLab:master.

@jacobtylerwalls
Copy link
Member Author

musicxml needs a bit more work -- something to do with making the tuplet brackets close in the right place.


stripTies is one way we would get these values, as in the Schoenberg, but also MIDI parsing, esp. because the default quantization units are 0.25 and 0.33, so it is easy to end up with streams like this:

>>> s = stream.Stream()
>>> s.insert(0, note.Note(quarterLength=(1/3)))
>>> s.insert(0.75, note.Note(quarterLength=(1/4)))
>>> s.makeRests(fillGaps=True, inPlace=True)
>>> s.show()

Before: (not ugly, but wrong)

Screen Shot 2021-01-09 at 2 20 05 PM

After: (ugly, mostly correct, but still need to get the first tuplet to end in the right place):

Screen Shot 2021-01-09 at 2 21 01 PM

And here's Schoenberg, ties stripped (and then created again on xml export):
Before:
Screen Shot 2021-01-09 at 2 29 44 PM

After:
Screen Shot 2021-01-09 at 2 29 50 PM

Source:
Screen Shot 2021-01-09 at 2 31 37 PM


The current approach runs makeTupletBrackets on the components, but the components possibly needs to be extracted and inserted far earlier in the process so that makeTupletBrackets can be re-run on the whole stream to get correct stop values.

@jacobtylerwalls jacobtylerwalls marked this pull request as draft January 9, 2021 19:35
@jacobtylerwalls jacobtylerwalls changed the title Apply tuplet to multiple components to express 5/6 or 7/3 QL [WIP] Apply tuplet to multiple components to express 5/6 or 7/3 QL Jan 9, 2021
@mscuthbert
Copy link
Member

mscuthbert commented Jan 9, 2021

Ah! All this comes as a necessary consequence of a refactor around music21 v.2 where components do not have their own tuplets. In the past it would have been possible to express this:

    >>> duration.quarterConversion(5/6)
     QuarterLengthConversion(components=(DurationTuple(type='16th', dots=0, quarterLength=0.25),
                                         DurationTuple(type='16th', dots=0, quarterLength=0.25),
                                         DurationTuple(type='16th', dots=0, quarterLength=0.25),
                                         DurationTuple(type='16th', dots=0, quarterLength=0.25),
                                         DurationTuple(type='16th', dots=0, quarterLength=0.25)),
                             tuplet=<music21.duration.Tuplet 3/2/eighth>)

as:

    >>> duration.quarterConversion(5/6)
     QuarterLengthConversion(components=(
                                         DurationTuple(type='eighth', dots=0, quarterLength=0.5),
                                         DurationTuple(type='16th', dots=0, quarterLength=0.25, tuplet=<music21.duration.Tuplet 3/2/eighth>),
                                         DurationTuple(type='16th', dots=0, quarterLength=0.25, tuplet=<music21.duration.Tuplet 3/2/eighth>)),
     )

But the (huge speed increase and huge simplification) change to DurationTuples (from DurationComponent objects) made this impossible.

What would be very helpful to put in this PR is a new attribute on the Duration object (and its slots)

def __init__(self, ...):
      ...
      self.expressionIsInferred = False

this has an analogy to Pitch's .spellingIsInferred -- meaning that exporters and transformers should feel free to reorganize the components or tuplets at any point rather than needing to respect them. And it should be set to True any time a Duration is set from QLs rather than type, dots, etc.

Then .splitAtComponents() might (unless a separate kwarg strictSplit=False is True) be able to eventually do something like:

    >>> n = note.Note(quarterLength=5/6)
    >>> n.duration.expressionIsInferred
    True
    >>> n.duration.components
    [ five 16th ]

    >>> [sub.duration for sub in n.splitAtComponents()] 
    [<duration ... normal eighth note with expressionIsInferred still True>,
      <duration ... 16th note triplet  with expressionIsInferred still True>,
      <duration ... 16th note triplet  with expressionIsInferred still True>]

and be able to reconnect the first three 16ths under the tuplet into one eighth note.

But with strict=True:

    >>> [sub.duration for sub in n.splitAtComponents(strict=True)] 
    [<duration ... 16th note triplet  with expressionIsInferred still True>,
     <duration ... 16th note triplet  with expressionIsInferred still True>,
     <duration ... 16th note triplet  with expressionIsInferred still True>,
     <duration ... 16th note triplet  with expressionIsInferred still True>,
     <duration ... 16th note triplet  with expressionIsInferred still True>]

From the strict=False example, later, we would be able to do fancy things like reconnect the final two 16th note triplets together if there is a 16th note triplet following it in the stream, etc. in the makeNotation or something like that.

It would move closer to a very-long-term-goal of getting this Stream...

    >>> s = stream.Stream([note.Note(ql=1/3), note.Note(ql=1/2), note.Note(ql=1/6) ]

...to express itself properly as containing a triplet of eighth, dotted eighth, sixteenth, rather than a triplet eighth - break tuplet - normal eighth - start tuplet again - triplet 16th.

None of that needs to go in this PR, but if expressionIsInferred can go in now, it'd make further work easier. My sense is that ideally any manual setting of type, dots, tuplet, (or ideally, components) would set expressionIsInferred to False. But we don't have that in for Pitch's spellingIsInferred yet either because it would make it so hard to do.

@jacobtylerwalls

This comment has been minimized.

@jacobtylerwalls

This comment has been minimized.

@jacobtylerwalls

This comment has been minimized.

@jacobtylerwalls jacobtylerwalls changed the title [WIP] Apply tuplet to multiple components to express 5/6 or 7/3 QL Apply tuplet to multiple components to express 5/6 or 7/3 QL; add splitAtDurations to makeNotation Jan 11, 2021
@jacobtylerwalls jacobtylerwalls marked this pull request as ready for review January 11, 2021 03:08
@jacobtylerwalls
Copy link
Member Author

jacobtylerwalls commented Jan 11, 2021

Well... closer....

Screen Shot 2021-01-10 at 10 29 31 PM

Update: this "extra" bracket is a consequence of the mixture of tuplet types: the sixteenths are 3/2/16th, and the eighth and quarters are 3/2/quarter. So makeNotation.makeTupletBrackets() will use a completionTarget of 0.5QL for the first 3/2/16th target, and then terminate the bracket once that target has been exceeded on the triplet-eighth, yielding 0.67QL.

I experimented with letting the bracket extend over the mixed tuplets, which I can get music21 to represent, but musicxml readers will vary. Here is the diff:

index c8d4309f1..eeafff764 100644
--- a/music21/stream/makeNotation.py
+++ b/music21/stream/makeNotation.py
@@ -1285,7 +1285,9 @@ def makeTupletBrackets(s, *, inPlace=False):
 
             # this, below, is optional:
             # if next normal type is not the same as this one, also stop
-            elif tupletNext is None or completionCount >= completionTarget:
+            elif (tupletNext is None
+                  or completionCount == completionTarget
+                  or tupletPrevious.tupletMultiplier() != tupletObj.tupletMultiplier()):
                 tupletObj.type = 'stop'  # should be impossible once frozen...
                 completionTarget = None  # reset
                 completionCount = 0  # reset
@@ -1480,4 +1482,6 @@ class Test(unittest.TestCase):
 
 if __name__ == '__main__':
     import music21
-    music21.mainTest(Test)
+    sch = music21.corpus.parse('schoenberg/opus19', 6)
+    sch.stripTies().show()
+    #music21.mainTest(Test)

Finale closing the bracket early even though the "stop" is not set until the last quarter note:

mixed-tuplets-under-single-bracket

MuseScore with the less presumptive rendering:

mixed-tuplets-bracket-musescore

@mscuthbert
Copy link
Member

Btw -- I am still thinking about this -- sorry it's been so slow to get on it. It's a big thought about whether something that was wrong but "looked good" is preferable to something that's right but looks really poor on output.

@jacobtylerwalls
Copy link
Member Author

Sounds good, no rush. Just tested a file against it and there's a bug -- corrupt XML output, so I can set this back to draft for now.

There have a been couple instances where I've thought .show() needs a makeNotation=False argument so that you can freeze what you have (for instance, if you've run makeNotation with splitAtDurations=False). That could be a way to accommodate both wrong-looking-good and right-looking-ugly scenarios.

@jacobtylerwalls jacobtylerwalls marked this pull request as draft February 8, 2021 20:56
@mscuthbert
Copy link
Member

show definitely needs a makeNotation=False. But before we do that, I'm going to create a new issue to help get keywords from getting out of hand.

@jacobtylerwalls jacobtylerwalls changed the title Apply tuplet to multiple components to express 5/6 or 7/3 QL Apply tuplet to multiple components to express 5/6 or 7/3 QL; add attribute expressionIsInferred Feb 15, 2021
@jacobtylerwalls jacobtylerwalls marked this pull request as ready for review February 15, 2021 23:43
@jacobtylerwalls
Copy link
Member Author

Here's what would suffice for a "don't make notation" argument (which doubles as "don't make a deepcopy")

index ecc445d38..9c6925e31 100644
--- a/music21/converter/subConverters.py
+++ b/music21/converter/subConverters.py
@@ -974,8 +974,15 @@ class ConverterMusicXML(SubConverter):
             defaults.title = ''
             defaults.author = ''
 
+        dataBytes: bytes = b''
         generalExporter = m21ToXml.GeneralObjectExporter(obj)
-        dataBytes: bytes = generalExporter.parse()
+        if ('Score' in obj.classes
+            and 'makeNotation' in keywords
+            and keywords['makeNotation'] is False):
+            # this keyword could have been wellFormed=True instead of makeNotation=False
+            dataBytes = generalExporter.parseWellFormedObject()
+        else:
+            dataBytes = generalExporter.parse()
 
         writeDataStreamFp = fp
         if fp is not None and subformats is not None:

Given that, do you want to wait until a larger keywords refactor is tackled before dropping this in?

@jacobtylerwalls
Copy link
Member Author

jotting a note for future self: above patch isn't enough, because fixupNotationFlat and measured also need to be skipped over

@jacobtylerwalls
Copy link
Member Author

jacobtylerwalls commented Mar 8, 2021

Hey @napulen happy to hear of your interest here. This ended up touching a lot of parts of the system, so here's my recap of where I think things stand right now:

  • do math to avoid resorting to non-power of 2 tuplets to express 5/6 QL and 7/3 QL etc, which you can get from ties and gaps (Non-power of 2 tuplet isn't the best choice for durations like 5/6 QL #572) -- done and tested in this PR, but not mergeable until we have a way to make it less ugly on export (all those tiny rests in the second screenshot, at the top)
  • redraw tuplet brackets when splitting on components -- done and tested in this PR
  • join tuplet brackets when the ratio is the same but type is different -- done and tested in this PR, but may not end up needing it depending on what we do below, and it may not even be a great idea, given that MuseScore is conveniently lax about these mixed type brackets but Finale is inconveniently strict. There's some more roubst logic for this stuff in TupletFixer which I haven't looked at, and Myke should weigh in.
  • .show() needs a way to skip making a copy and running makeNotation (Add makeNotation=False option to musicxml export and emit warnings on malformed streams #996)
  • do some magic to break into tuplets or consolidate from tuplets based on context (Express durations with or without tuplets according to context #904). needing to consolidate is why we haven't merged this PR yet, and needing to break into tuplets is your case with the Hensel pieces
  • new expressionIsInferred attribute on Durations to track whether it's okay to do magic -- done and tested in this PR, but should probably be merged separately

So what's left is the magic ✨

For a little proof of concept for 1/3 QL - 1/3 QL - 1 QL (rest) - 1/3 QL I would think it would go like:

  • Do a scan through the notesAndRests (GeneralNote) of a measure or voice
  • Is there an incomplete tuplet? Grab the .next() GeneralNote, but only if .expressionIsInferred
  • Re-express that one if it helps, and set .components
  • Then do a second pass from the beginning, and as long as expressionIsInferred, consolidate groups that are unnecessarily expressed as a tuplet (like the second screenshot at the top).

I don't want to take all the fun, so if you want to pair or split some of this up, send me a note.

@napulen
Copy link
Contributor

napulen commented Mar 10, 2021

Hey @jacobtylerwalls, thanks for the great summary.

Do you have a timeline for this? I want to go for the Humdrum voice issue first. Just to know when are you expecting to finish this PR.

@jacobtylerwalls
Copy link
Member Author

I would say May is the most likely point for me to dig in to the hard tasks here. I could probably get the expressionIsInferred bits and the don't makeNotation bits merged in smaller PRs first. So yeah, plenty of time to focus on Humdrum!

@jacobtylerwalls jacobtylerwalls changed the title Apply tuplet to multiple components to express 5/6 or 7/3 QL; add attribute expressionIsInferred Apply tuplet to multiple components to express 5/6 or 7/3 QL Mar 12, 2021
@jacobtylerwalls jacobtylerwalls changed the title Apply tuplet to multiple components to express 5/6 or 7/3 QL WIP: Apply tuplet to multiple components to express 5/6 or 7/3 QL May 7, 2021
@jacobtylerwalls
Copy link
Member Author

Pieces of this were merged in other PRs, but the main idea -- applying a tuplet to multiple components -- can be more easily handled in a new PR once #904 is addressed; this one no longer applies cleanly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non-power of 2 tuplet isn't the best choice for durations like 5/6 QL
4 participants