Auto-estimate margins based on freq_min/f0+Q for filters #4227
Conversation
|
I think this is a great feature, but I already told @samuelgarcia that maybe we should at very least warn (prevent ?) users that will use very low frequencies for filters, trying to get LFP and/or low pass versions of the data. If the automatic margin starts to be really large (maybe larger than the default chunk size of 1s), then we should warn users that they are doing something clearly suboptimal. |
Sub-optimal but somehow needed! Maybe we can warn if the margin is greater than the |
|
Yes, I've read the PR, and indeed, at least a warning that the theoretical margin should be XX, and that it has be clipped to max_margin_s. The thing is that if users really want to do that, then the warning message could also advise them to increase chunk_duration in the default_job_kwargs, to reduce IO overloads |
|
We democratically decided to remove the max_margin kwargs and instead make a mechanism that raise an error if the end-user try to filter in LFP band and then point to a link to a clear and didactic and really exemplar tutorial about chunking and its bad effect on low freqs. |
|
This is ready for final review. here's a new doc page explaining the issue: https://spikeinterface--4227.org.readthedocs.build/en/4227/how_to/extract_lfps.html |
| # **Why does this fail?** | ||
| # | ||
| # The error occurs because by default in SpikeInterface when highpass filtering below 100 Hz. | ||
| # Filters with very low cutoff frequencies have long impulse responses, which require larger margins to avoid edge artifacts between chunks. |
There was a problem hiding this comment.
I think we need to explain what a margin and a chunk is more clearly before this?
If I were writing it, I'd have a first section called "1. Chunks and margins" and explain what chunking is, why we do it, and what a margin is. Or put in more explanation here.
There was a problem hiding this comment.
Oh, I've read more now... so I would put Section 3 earlier in the tutorial. And put in a longer description of a chunk and margin.
There was a problem hiding this comment.
added at the top of the file
|
Hello, amazing! You could set this doc up as a jupyter notebook that gets run in the CI (e.g. https://github.com/SpikeInterface/spikeinterface/tree/main/examples/tutorials/forhowto) Then we don't have to do all the .py -> .rst conversion annoying stuff. |
Done! Also moved the |
|
@chrishalcrow needed some juggling to make it work with RTD RAM (the script used to save everything in memory), but it works now! Let me know if you have more comments: https://spikeinterface--4227.org.readthedocs.build/en/4227/forhowto/plot_extract_lfps.html |
| ############################################################################## | ||
| # **Why does this fail?** | ||
| # | ||
| # The error occurs because by default in SpikeInterface when highpass filtering below 100 Hz. |
There was a problem hiding this comment.
Current sentence doesn't make sense. Maybe:
| # The error occurs because by default in SpikeInterface when highpass filtering below 100 Hz. | |
| # The error always occurs in SpikeInterface when highpass filtering below 100 Hz, to remind the user that they need to be careful. |
?
| # a "margin" (extra samples at the edges) to avoid edge artifacts when filtering. Let's demonstrate | ||
| # this by saving the filtered data with different chunking strategies. | ||
| # | ||
| # **This error is to inform the user that extra care should be used when dealing with LFP signals!** |
There was a problem hiding this comment.
The start of this section is a bit confusing. This line refers to the error from the previous section -- but since we're in a new section I think the reader will be confused. Maybe move this statement to just after the error?
| # | ||
| # We can ignore this error, but let's make sure we understand what it's happening. | ||
|
|
||
| # We can ignore this error, but let's see what is happening |
There was a problem hiding this comment.
This line and the line above repeat
| print(f"Margin: {margin_in_s} s") | ||
|
|
||
| ############################################################################## | ||
| # This effectively means that if we plot 1-s snippet of traces, a total of 11 s will actually be read and filtered. |
There was a problem hiding this comment.
I think it's better to mention the overhead here, since this sentence explains very clearly why there is a big overhead. E.g.
| # This effectively means that if we plot 1-s snippet of traces, a total of 11 s will actually be read and filtered. | |
| # This effectively means that if we plot 1-s snippet of traces, a total of 11 s will actually be read and filtered. Hence the computational "overhead" is very large. |
| _ = sw.plot_traces(recording_filt, time_range=[20, 21]) | ||
|
|
||
| ############################################################################## | ||
| # A warning tells us that what we are doing is not optimized, since in order to get the requested traces |
There was a problem hiding this comment.
There's no warning in the output now, sorry, probably something to do with changing how it's rendered?
There was a problem hiding this comment.
I do see the warning locally, but not on the build docs...not sure why! Should I just remove it?
| ############################################################################## | ||
| # From the plot, we can see that there is a very small error when the margin size is large (green), | ||
| # a larger error when the margin is smaller (orange) and a large error when the margin is small (blue). | ||
| # So we need large margins (compared to the chunk size) if we want accurate filtered. |
There was a problem hiding this comment.
Apart from the lil things, this is great - I'm happy!
| ftype="butter", | ||
| filter_mode="sos", | ||
| margin_ms=5.0, | ||
| margin_ms=5, |
There was a problem hiding this comment.
| margin_ms=5, | |
| margin_ms=5.0, |
4 participants
@samuelgarcia let's discuss!