-
Notifications
You must be signed in to change notification settings - Fork 148
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
Improve performance of expo by multiplying previous result #103
base: master
Are you sure you want to change the base?
Conversation
This seems good to me. Just curious, did you run into this because you in your use case the number of retries was very high? |
No, I was just browsing the code and noticed that some work was unnecessarily re-done on each loop. I think it's pretty unlikely that anyone would reach enough retries for this to actually make much of a difference, but I figured I'd submit it anyway since it's such a simple change. |
e11e666
to
28552d5
Compare
Any chance this can get merged? PR has been open for over a year. |
Ping? |
while True: | ||
a = factor * base ** n | ||
if max_value is None or a < max_value: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This max_value
condition doesn't work properly with your changes, it needs to be updated a bit, like this, but still 24x faster than the original:
base_n = 1
while True:
a = factor * base_n
if max_value is None or a < max_value:
yield a
base_n *= base
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks.
28552d5
to
651a453
Compare
This computes the exponential by multiplying the previous result by
base
instead of computing it from scratch with**
each time, which seems to be about an order of magnitude faster. On my machine the following tests report around 2.63 seconds forexpo_old
and 0.10 forexpo_new
.