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

estimate_symbolic_duration breaks for some unknown durations #371

Open
sildater opened this issue Sep 2, 2024 · 3 comments · Fixed by #372
Open

estimate_symbolic_duration breaks for some unknown durations #371

sildater opened this issue Sep 2, 2024 · 3 comments · Fixed by #372
Assignees
Labels
bug Something isn't working invalid This doesn't seem right

Comments

@sildater
Copy link
Member

sildater commented Sep 2, 2024

loading beethoven_op053_mv3.match from the Zeilinger dataset throws an error in estimate_symbolic_duration. The inputs to reproduce without the file are pt.utils.estimate_symbolic_duration(2496, 96). Looking into this, for most smaller fractions returns nonsensical values (like {'type': 'long', 'actual_notes': 1, 'normal_notes': 2}) if it doesn't hit a symbolic duration, for larger fractions the method fails. Test script:

not_working = 0
divs = 12
quarters = 32
for k in range(divs * 0, divs * quarters):
    try:
        a = pt.utils.estimate_symbolic_duration(k, divs)
        print(k, k/divs, a)
    except:
        print("not working:", k, k/divs)
        not_working +=1
print("percentage not working",not_working / (divs * quarters))
@sildater sildater added bug Something isn't working invalid This doesn't seem right labels Sep 2, 2024
@manoskary manoskary self-assigned this Sep 4, 2024
@manoskary
Copy link
Member

What should the expected duration or the intended behavior be?
I guess before partitura=1.5.0 when you had a failed case, the duration was assigned to None.
In the example you mention, the resulting quarter duration of 2496 / 96would be 26 quarter notes.
26 quarter notes should be a composite duration, i.e. a tied note. In the latest version of estimate_symbolic_durations, we can handle composite durations but our vocabulary is still limited. To guess composite duration you need to set estimate_symbolic_duration(return_com_durations=True) then the output of the functions can be a list of dicts.

Currently, the estimate_symbolic_duration function has three clauses, actual possible durations, some kind of triplet duration, or composite durations. The choice was made so that the function does not return NoneValue and therefore is forced to choose a duration. Of course, it is very very far from being perfect with the idea being we keep adding new encountered durations as we find errors. Furthermore, the function is required to not fail so it can be able to guess arbitrary triplet durations, but as you've seen this does not work correctly in particular when composite durations are involved.

Another solution would be to allow the function to return None durations or empty dicts then whenever we cannot give a credible answer then we do not give any answer. I would be very open to having a deeper discussion on this topic to better structure the estimation so it could cover as many cases as possible and fit everybody's needs.

@sildater
Copy link
Member Author

sildater commented Sep 4, 2024

Thanks for looking into this! I think the intended behavior should be to a) not raise exceptions and b) not give wrong values. None is good return value instead of the triplet condition which is the buggy part.

@manoskary
Copy link
Member

Ok, I think I have a solution, I will add you as the reviewer.

manoskary added a commit that referenced this issue Sep 5, 2024
@manoskary manoskary linked a pull request Sep 5, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working invalid This doesn't seem right
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants