Alternative implementation of causal filter in filter.py #3133
Conversation
Co-authored-by: Alessio Buccino <[email protected]>
Co-authored-by: Alessio Buccino <[email protected]>
Co-authored-by: Alessio Buccino <[email protected]>
Co-authored-by: Alessio Buccino <[email protected]>
Co-authored-by: Alessio Buccino <[email protected]>
|
@JuanPimientoCaicedo can you make a PR from the The problem is that I cannot push to your main branch, but if you make a PR from a different branch I can...and there are a few things to fix (see failing tests) |
|
Otherwise you can fix it yourself here. You can just move the new parameters in the |
|
@JuanPimientoCaicedo I was actually able to commit from GitHub :) @samuelgarcia this looks good to me! |
for more information, see https://pre-commit.ci
| @@ -291,14 +333,77 @@ def __init__(self, recording, freq=3000, q=30, margin_ms=5.0, dtype=None): | |||
| self._kwargs = dict(recording=recording, freq=freq, q=q, margin_ms=margin_ms, dtype=dtype.str) | |||
|
|
|||
|
|
|||
| class CausalFilter(FilterRecording): | |||
There was a problem hiding this comment.
Why having this class if the FilterRecording already handle it with specific options ?
There was a problem hiding this comment.
I guess it is for the same reason highpass_filter and bandpass_filter exist. Although it is true that in causal filtering, the users might need to provide some extra parameters compared with these other subclasses...
There was a problem hiding this comment.
I agree with @samuelgarcia we don't need the class in this case, since the CasualFilter class is really just instantiating the FilterRecording class (the fix_dtype and kwargs will be handled by the FilterRecording class).
My suggestion is to:
- remove this class
- explicittly define the
causal_filterfunction (see my comment)
|
Should it be added to the rest in the rst in the docs? |
There was a problem hiding this comment.
@JuanPimientoCaicedo some other requests.
Also, can you add some tests?
| @@ -291,14 +333,77 @@ def __init__(self, recording, freq=3000, q=30, margin_ms=5.0, dtype=None): | |||
| self._kwargs = dict(recording=recording, freq=freq, q=q, margin_ms=margin_ms, dtype=dtype.str) | |||
|
|
|||
|
|
|||
| class CausalFilter(FilterRecording): | |||
There was a problem hiding this comment.
I agree with @samuelgarcia we don't need the class in this case, since the CasualFilter class is really just instantiating the FilterRecording class (the fix_dtype and kwargs will be handled by the FilterRecording class).
My suggestion is to:
- remove this class
- explicittly define the
causal_filterfunction (see my comment)
|
|
||
| bandpass_filter.__doc__ = bandpass_filter.__doc__.format(_common_filter_docs) | ||
| highpass_filter.__doc__ = highpass_filter.__doc__.format(_common_filter_docs) | ||
| causal_filter.__doc__ = causal_filter.__doc__.format(_common_filter_docs) |
There was a problem hiding this comment.
| causal_filter.__doc__ = causal_filter.__doc__.format(_common_filter_docs) |
We don't need this anymore since we redefined the docstring
|
Also you should add the new function to the API rst. Again, I think the best would be for you to make a new PR NOT from the main branch, so we can contribute to it. |
Co-authored-by: Alessio Buccino <[email protected]>
for more information, see https://pre-commit.ci
Co-authored-by: Alessio Buccino <[email protected]>
for more information, see https://pre-commit.ci
|
closed in favor of #3172. |
Sorry for the delay, guys. As requested by @samuelgarcia, this is another way to implement causal filtering in filter.py.
Let me know what you think.
PR related to: #2942