Added filterbanking #158
Added filterbanking #158
Conversation
| frequency_bands: list(tuple) | ||
| The frequency bands to be fitlered for (e.g. [(4, 8), (8, 13)]) | ||
| drop_original_signals: bool | ||
| Whether to drop the original unfiltered signals |
There was a problem hiding this comment.
In the docstring above I stated to refer to mne.filter.filter_data() for a description of the parameters. Should I copy the docstring from mne instead and paste it here?
|
I would say yes. I expect to learn about parameters in the Parameters
section. Even **filter_kwargs should be explained there.
… |
braindecode/datautil/preprocess.py
Outdated
| # copy docstring from mne to here | ||
| filterbank.__doc__ += mne.io.Raw.filter.__doc__[ | ||
| mne.io.Raw.filter.__doc__.find("picks : "): | ||
| mne.io.Raw.filter.__doc__.find("Returns")].replace(8 * " ", 4 * " ") |
There was a problem hiding this comment.
Honestly this is black magic. This code in mne barely changes so I would copy the docstring.
YOu can make a test that asserts that docs are in sync. It's much simpler.
There was a problem hiding this comment.
I feel previous solution to explicitly refer to MNE also has advantages:
- Less clutter in our code
- It is more explicit that we just forward these arguments, one does not have to check whether there is any difference between MNE and us
And we anyways build so much around MNE now, pretty much encouraging the user to learn some MNE.
braindecode/datautil/preprocess.py
Outdated
|
|
||
| def filterbank(raw, frequency_bands, drop_original_signals=True, n_jobs=1, | ||
| method='fir', iir_params=None, phase='zero', fir_window='hamming', | ||
| fir_design='firwin', **filter_kwargs): |
There was a problem hiding this comment.
why not simply having **filter_kwargs and say it supports params of filter function from mne?
if feels you explicit some params and some not.
There was a problem hiding this comment.
In another PR you mentioned disliking kwargs and to make arguments explicit. I therefore took the arguments that are relevant for my use case and made them explicit, while at the same time keeping full compatibility with mne through filter_kwargs. I will now update it again to be less explicit and only have the filter_kwargs. The docstring for this parameter will then contain a referral to mne.
There was a problem hiding this comment.
arfff to be it's one option or the other. If it was me and I would copy all
the mne parameters and copy the docstring. After updating the docstring
5 times due to mne changes I would reconsider. Note that mne release
are every 6 months so...
…cky updating of docstring.
Filterbanking can now be used similarly to the other preprocessing steps.
It modifies a
mne.io.Rawin-place and adds new channels with the filtered signals.