Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Alternative implementation of causal filter in filter.py #3133
Alternative implementation of causal filter in filter.py #3133
Changes from 10 commits
f5cbc4d
e595458
500410f
e74f5c2
1a7149d
c6e3cbc
377808a
ef402be
892b7b6
bd4909f
25d25fd
7c5956e
b5095ee
3f1024b
1d03d5e
3efa5f5
8c10bcc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why having this class if the FilterRecording already handle it with specific options ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @samuelgarcia we don't need the class in this case, since the
CasualFilter
class is really just instantiating theFilterRecording
class (thefix_dtype
andkwargs
will be handled by theFilterRecording
class).My suggestion is to:
causal_filter
function (see my comment)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this anymore since we redefined the docstring