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

[ENH] Speed up _index_to_events #50

Closed
snwnde opened this issue Dec 23, 2021 · 5 comments · Fixed by #51
Closed

[ENH] Speed up _index_to_events #50

snwnde opened this issue Dec 23, 2021 · 5 comments · Fixed by #51
Assignees
Labels
enhancement 🚧 New feature or request

Comments

@snwnde
Copy link
Contributor

snwnde commented Dec 23, 2021

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:

def _index_to_events(x):
    x_copy = np.copy(x)
    x_copy[:,1] += 1 
    split_idx = x_copy.reshape(-1).astype(int)
    full_idx = np.arange(split_idx.max())
    index = np.split(full_idx, split_idx)[1::2]
    index = np.concatenate(index)
    return index

Please tell me how you think of it. If (pre)approved I could create a PR :)

@snwnde snwnde changed the title [ENH] Speed up _index_to_events [ENH] Speed up _index_to_events Dec 23, 2021
@raphaelvallat raphaelvallat self-assigned this Dec 23, 2021
@raphaelvallat raphaelvallat added the enhancement 🚧 New feature or request label Dec 23, 2021
@raphaelvallat
Copy link
Owner

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

@snwnde
Copy link
Contributor Author

snwnde commented Dec 24, 2021

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)

snwnde added a commit to snwnde/yasa that referenced this issue Dec 24, 2021
@raphaelvallat
Copy link
Owner

That's great! And the output of these two functions is always the same, correct?

@raphaelvallat raphaelvallat linked a pull request Dec 26, 2021 that will close this issue
@snwnde
Copy link
Contributor Author

snwnde commented Dec 28, 2021

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.

@raphaelvallat
Copy link
Owner

That it is indeed unrelated. Merging the PR now! Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🚧 New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants