Closed snwnde closed 2 years ago
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, Raphael
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
, where my_index_to_events
is my implementation:
%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?
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 :)