-
Notifications
You must be signed in to change notification settings - Fork 116
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
[ENH] Speed up _index_to_events
#50
Comments
_index_to_events
Hi @snwnde, Thanks for the PR, appreciate it! This looks great, but have you tested that it actually improves speed on say, 100, 500, 1000 and 5000 events? Would be good to have some benchmarks here for future reference. If it does, then please feel free to draft a PR! Thanks, |
For events generated this way, test_starts_NUM = np.arange(NUM) * 50
test_ends_NUM = test_starts_NUM + np.random.randint(0, 30)
test_index_NUM= np.vstack((test_starts_NUM, test_ends_NUM)).T I obtained the following results from %timeit _index_to_events(test_index_100)
583 µs ± 9.06 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
%timeit my_index_to_events(test_index_100)
356 µs ± 14.6 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
%timeit _index_to_events(test_index_500)
3.5 ms ± 54.8 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
%timeit my_index_to_events(test_index_500)
1.6 ms ± 16.6 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
%timeit _index_to_events(test_index_1000)
6.89 ms ± 62.5 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
%timeit my_index_to_events(test_index_1000)
3.29 ms ± 50.9 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
%timeit _index_to_events(test_index_5000)
61.2 ms ± 968 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
%timeit my_index_to_events(test_index_5000)
17.3 ms ± 190 µs per loop (mean ± std. dev. of 7 runs, 100 loops each) |
That's great! And the output of these two functions is always the same, correct? |
@raphaelvallat Sure! In my tests they are always the same. Anyway, I believe this must be confirmed by the test codes. I see in the PR page that all tests passed except the one for Windows python 3.6 build. A closer look at the failed one's logs suggests it's the problem of the test environment itself. |
That it is indeed unrelated. Merging the PR now! Thanks again! |
The function
_index_to_events
in others.py could be implemented in another fashion, avoiding the use of a for loop.Here is my proposal:
Please tell me how you think of it. If (pre)approved I could create a PR :)
The text was updated successfully, but these errors were encountered: