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

[MRG] Add multidim decorator to filter_signal #264

Merged
merged 3 commits into from
Jul 30, 2021
Merged

Conversation

ryanhammonds
Copy link
Member

@ryanhammonds ryanhammonds commented Jun 15, 2021

This allows 2d signals to be passed to filter_signal. My use case was filtering a 2d array of signals from a MEA.

I also updated np.stack to np.array in the decorator since it's equivalent in this case and 10x faster.

%%timeit
out = [i+j for i in range(1000) for j in range(1000)]
out = np.stack(out)

1.27 s ± 10.6 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

%%timeit
out = [i+j for i in range(1000) for j in range(1000)]
out = np.array(out)

98.9 ms ± 304 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

@TomDonoghue
Copy link
Member

This definitely falls under the category of "poorly documented behaviour", but I think the reason that the multidim decorator wasn't added to the filter functions is that these functions actually do already work on 2d signals.

On main, for me, this currently works:

data_2 = np.ones([10, 1000])
out2 = filter_signal(data_2, 1000, 'bandpass', (8, 12))

The most relevant step for this is that the apply_* filter functions need to work on 2d arrays, and as set up I think they do, based on how they work with numpy arrays. In this case, not having the multidim decorator is preferable, if we can avoid it, since multidim would call the function for each row, redundantly regenerating the filter each time, whereas as long as the apply function can apply the filter across all rows, then everything else only needs to be executed once.

@ryanhammonds - were you running into some error with applying the filter to 2d arrays? If not, and you can have a quick check that the current approach does generalize beyond 1d, then I think what we should do here is update the docs, rather than add the decorator.

Also: woah on the different between stack and array. That's wild. Good catch! We should update that.

@ryanhammonds
Copy link
Member Author

ryanhammonds commented Jun 29, 2021

@TomDonoghue Thanks for this! You're totally correct, it works for 1d or 2d (but not 3d) signals. I don't see a huge use case for 3d arrays anyways. And like you mentioned it's redundant. I removed the multidim decorator, but did add a note in the docstring that 2d arrays are supported. I also added a small test to show it's does indeed work with 2d arrays as intended.

I was surprised by the the performance diff of stack vs array as well. I think in most cases np.array + list comprehension is typically faster than stack/vstack. I have a weird fascination with code optimization, maybe I should learn C / other lower level language some day lol.

@TomDonoghue TomDonoghue added the 2.2 Updates to go into a 2.2.0 release label Jul 30, 2021
@ryanhammonds ryanhammonds merged commit 50a9360 into main Jul 30, 2021
@ryanhammonds ryanhammonds mentioned this pull request Jul 30, 2021
26 tasks
@ryanhammonds ryanhammonds deleted the multidim_filt branch July 30, 2021 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.2 Updates to go into a 2.2.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants