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

speed up sortTuple for streams #1624

Merged
merged 1 commit into from
Jun 27, 2023
Merged

Conversation

TimFelixBeyer
Copy link
Contributor

This avoids the computation of the duration attribute when creating a stream's sortTuple, which is expensive as it requires iterating over all elements to compute highestTime.
Yields significant speedups on tasks that require sorting (12% overall for a simple load + quantize benchmark on my machine).

This avoids the computation of the duration attribute when creating a stream's `sortTuple`, which is expensive as it requires iterating over all elements to compute `highestTime`.
@coveralls
Copy link

Coverage Status

coverage: 92.986% (-0.001%) from 92.987% when pulling a184f77 on TimFelixBeyer:patch-14 into 8ab0fe9 on cuthbertLab:master.

@mscuthbert
Copy link
Member

Yes! This is significant!

I had the idea at some point of a GraceStream which was a stream entirely of grace notes for things like complex ornaments that have beams and their own accent patterns etc which take up zero conceptual time. But I don't think I ever implemented that and if I did then we can add a special case.

Streams are one of the few subclasses of Music21Object that the base class is allowed to be aware of (that and Spanner) so putting this code in the main class is okay.

I'll merge but please note that i'm less of a fan of "x = 1 if ...compound-tests-that-are-long... else 0" type of expressions rather than the simple if x: ... else: ... type. I try to avoid them except in generators/comprehensions/lambdas. And even then only if the normal case is left of the if.

@mscuthbert mscuthbert merged commit 4533062 into cuthbertLab:master Jun 27, 2023
@TimFelixBeyer
Copy link
Contributor Author

I’ll keep that in mind for future PRs!

@TimFelixBeyer TimFelixBeyer deleted the patch-14 branch June 28, 2023 00:19
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.

3 participants