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

Apply tuplet to multiple components to express durations like 5/6 or 7/3 QL #1240

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

jacobtylerwalls
Copy link
Member

@jacobtylerwalls jacobtylerwalls commented Mar 1, 2022

Fixes #572
Same PR as #763, just with conflicts fixed and commits squashed

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.


@gregchapman-dev would you be able see if it helps with your makeRests() issues?

This is not mergeable until we have a solution for #904. I have a diff for that I should be able to PR in a week or so--still testing it. Edit: done!

@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 durations like 5/6 or 7/3 QL Mar 1, 2022
@coveralls
Copy link

coveralls commented Mar 1, 2022

Coverage Status

coverage: 93.023% (-0.02%) from 93.047% when pulling a825cd2 on jacobtylerwalls:components-2 into cddb104 on cuthbertLab:master.

@gregchapman-dev
Copy link
Contributor

gregchapman-dev commented Mar 1, 2022

Doesn't work yet, but it's trying hard! Looks like the loop that creates all the components is just creating component after component, each with type smaller than the last, until it hits 8 iterations and gives up. Here's my test code snippet (note that your loop doesn't run until the first print, because of lazy components update):

# test PR #1240
restVoice = m21.stream.Voice()
positionedRest = m21.note.Rest()
positionedRest.duration = m21.duration.Duration(Fraction(1, 3))
restVoice.insert(Fraction(11, 3), positionedRest)
restVoice.makeRests(inPlace=True,
                    hideRests=True)

for i, rest in enumerate(restVoice.notesAndRests):
    print(f'rest[{i}] = {rest}')
    print(f'rest[{i}].duration = {rest.duration}')
    print(f'rest[{i}] tuplets = {rest.duration.tuplets}')
    print(f'rest[{i}] components = {rest.duration.components}')

music21/duration.py Outdated Show resolved Hide resolved
@gregchapman-dev
Copy link
Contributor

gregchapman-dev commented Mar 2, 2022 via email

@jacobtylerwalls
Copy link
Member Author

Yeah, this looks usable for my case, although I really was hoping for a half rest, a quarter rest, and then the one eighth-rest triplet I really need.

Right, that's next in #904. Hoping to ping you for a test then also :-)

@gregchapman-dev
Copy link
Contributor

gregchapman-dev commented Mar 2, 2022 via email

@mscuthbert
Copy link
Member

This is looking good, but I don't think I want to go forward w/o allowing for the current usage as well, which is often seen in certain new complexity scores. I'd prefer only to merge when the #904/merging fix is also in, because I'm not sure that 11 rests for one note is definitely better than one strange tuplet in all cases.

@mscuthbert
Copy link
Member

Let me know when this is ready for a review.

@jacobtylerwalls
Copy link
Member Author

It's ready 🏁

Comment on lines +2716 to +2717
# Do this again, since makeNotation() might create complex rests
self.stream = self.stream.splitAtDurations(recurse=True)[0]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fairly rare to go through fixupNotationFlat(), since even "flat" scores go through a "general object conversion". You'd need a not-well-formed score or makeNotation=False to end up here. So that's why I thought this was not terrible to do this twice (better than erroring out with complex duration failures).

This comment was marked as outdated.

This comment was marked as outdated.

@jacobtylerwalls

This comment was marked as outdated.

@mscuthbert
Copy link
Member

Sorry I ahven't had a chance to look at this -- will try to get to it this week.

music21/duration.py Outdated Show resolved Hide resolved
music21/duration.py Outdated Show resolved Hide resolved
music21/duration.py Outdated Show resolved Hide resolved
@mscuthbert
Copy link
Member

Hi Jacob -- let me know if after everything this is still relevant -- this is one of the few PRs that I'll merge during sabbatical. Thanks!

music21/duration.py Outdated Show resolved Hide resolved
@jacobtylerwalls
Copy link
Member Author

Yep, it's still solving the problem of a 5/6QL note looking bizarre when exported.

Happy to give this a once-over myself for style/readability, but it should be ready for review as of now. Thanks for offering to look.

@mscuthbert
Copy link
Member

I guess my biggest concern was and remains that I'm not so sure that:

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

is superior to:

    >>> duration.quarterConversion(7/3)
    QuarterLengthConversion(components=(DurationTuple(type='whole', dots=0, quarterLength=4.0),),
        tuplet=<music21.duration.Tuplet 12/7/16th>)

even though the tuplet is way complex, there's something better about returning one note than seven.

I think I'd merge immediately if the output were something like this:

    >>> duration.quarterConversion(7/3)
    QuarterLengthConversion(components=(DurationTuple(type='half', dots=0, quarterLength=2.0),
                                        DurationTuple(type='quarter', dots=0, quarterLength=1.0),
                                        DurationTuple(type='eighth', dots=0, quarterLength=0.5)),
                            tuplet=<music21.duration.Tuplet 3/2/eighth>)

or even:

    >>> duration.quarterConversion(7/3)
    QuarterLengthConversion(components=(DurationTuple(type='half', dots=2, quarterLength=3.5),
                            tuplet=<music21.duration.Tuplet 3/2/eighth>)

And for this one:

    >>> 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/16th>)

we might see:

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

Would this work in the system? Either of these look superior to the current output and proposed new output.

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