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

Refactor fractionSum for simplicity and performance #1674

Closed
wants to merge 4 commits into from

Conversation

alexandermorgan
Copy link
Contributor

Important: I wasn't able to get the current version of the master branch to pass the tests even without any changes (perhaps I was doing something wrong). So this PR was made on top of the fix-mypy-error branch. That means that accepting this PR will also merge the fix-mypy-error branch changes.
About the actual changes I made, they're all in music21/meter/tools.py -> fractionSum. fractionSum is refactored to be easier to read and more performant. The same results are returned in the same format, but there's between a 0% and 50% decrease in runtime depending on the case. I didn't measure memory usage but it should also be lower since fewer iterables are created.
If this sort of PR is wanted, I have about 10 small changes (even smaller than this one) ready to go that I could also contribute. If so, is it better to commit each one individually, or one altogether? I dove into python sets and other basic structures and wanted to apply that knowledge in a practical way, and that is common thread in the changes.

@coveralls
Copy link

coveralls commented Dec 26, 2023

Coverage Status

coverage: 93.05% (-0.001%) from 93.051%
when pulling f0f422c on alexandermorgan:master
into 970624c on cuthbertLab:master.

@mscuthbert mscuthbert closed this Jan 2, 2024
@mscuthbert mscuthbert reopened this Jan 2, 2024
@mscuthbert
Copy link
Member

Can you explain how you are sure that your answer produces the exact same result as the previous code? I see that you are doing a lot of floating point arithmetic and then a round() at the very end. That's exactly the type of arithmetic that introduces errors in fractional arithmetic.

I can immediately find errors that didn't exist before:

x = []
for i in range(1, 1001):
     x.append((1, i))
fractionSum(tuple(x))

Old:
(53362913282294785045591045624042980409652472280384260097101349248456268889497101757506097901985035691409088731550468098378442172117885009464302344326566022502100278425632852081405544941210442510142672770294774712708917963967779610453224692426866468888281582071984897105110796873249319155529397017508931564519976085734473014183284011724412280649074307703736683170055800293659235088589360235285852808160759574737836655413175508131522517,
7128865274665093053166384155714272920668358861885893040452001991154324087581111499476444151913871586911717817019575256512980264067621009251465871004305131072686268143200196609974862745937188343705015434452523739745298963145674982128236956232823794011068809262317708861979540791247754558049326475737829923352751796735248042463638051137034331214781746850878453485678021888075373249921995672056932029099390891687487672697950931603520000)

New:

--> 267 n = round(total * d)
    268 return (n, d)

OverflowError: int too large to convert to float

If the rebuttal is "well, we'll never have so many time signatures to add to have this problem" then we'll never realize any performance improvements with this.

The general rule is - never cast a fraction to a float. It will always have some error on some OS that will never be detected. This is why fraction arithmetic is slower than this method, it's because it never has to worry about rounding errors.

@mscuthbert mscuthbert closed this Jan 2, 2024
@mscuthbert
Copy link
Member

Thanks for contributing though. I appreciate it. If doing performance-related boosts, be sure to focus attention on parts of the system that we're seeing performance problems on; I don't think that fractionSum is one of them, as it is very rarely called.

For PRs, best to make a number of small ones that each fix or improve one thing, unless they depend on each other somehow.

@mscuthbert
Copy link
Member

Safer speedups are included in #1631

@alexandermorgan
Copy link
Contributor Author

Understood and I appreciate the feedback. I'll submit more straight-forward pr when I have time.

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