Skip to content

Add low-pass filter to find_bad_channels_maxwell()#7983

Merged
larsoner merged 5 commits intomne-tools:masterfrom
hoechenberger:find-bads-filter
Jul 9, 2020
Merged

Add low-pass filter to find_bad_channels_maxwell()#7983
larsoner merged 5 commits intomne-tools:masterfrom
hoechenberger:find-bads-filter

Conversation

@hoechenberger
Copy link
Copy Markdown
Member

@hoechenberger hoechenberger commented Jul 9, 2020

What does this implement/fix?

The current approach to using find_bad_channels_maxwell() requires users to manually low-pass filter their data before passing it to the function. This is kind of cumbersome.

This PR, therefore, adds a h_freq kwarg that sets the cutoff frequency of an automatically-applied low-pass filter. Passing h_freq=None disables filtering. If the input data has already been low-pass filtered, a helpful warning is emitted.

We already have a similar API in preprocessing.compute_proj_ecg(), so this change makes things more consistent within the preprocessing module.

@hoechenberger
Add low-pass filter to find_bad_channels_maxwell()
7390422 
The current approach to using `find_bad_channels_maxwell()` requires
users to manually low-pass filter their data before passing it to the
function. This is kind of cumbersome.

This commit, therefore, adds a `h_freq` kwarg that sets the cutoff
frequency of an automatically-applied low-pass filter. Passing
`h_freq=None` disables filtering. If the input data has already been
low-pass filtered, a helpful warning is emitted.

We already have a similar API in `preprocessing.compute_proj_ecg()`, so
this change makes things more consistent within the `preprocessing`
module.
agramfort
agramfort reviewed Jul 9, 2020
View reviewed changes
mne/preprocessing/maxwell.py Outdated
.. versionadded:: 0.20
"""
if h_freq is not None:
if 'lowpass' in raw.info:
Copy link
Copy Markdown
Member

@agramfort agramfort Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lowpass is always present but it can be None. I think you will warn all the time here.

What I would do is something like this:

if raw.info.get('lowpass') and raw.info['lowpass'] < 'lowpass'

Copy link
Copy Markdown
Member Author

@hoechenberger hoechenberger Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed things in 94e5807, can you have a look?

agramfort
agramfort approved these changes Jul 9, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@larsoner thoughts on this?

@larsoner
Copy link
Copy Markdown
Member

larsoner commented Jul 9, 2020

-1 on this. There are lots of options for filtering. I don't see why we want to add this here

@larsoner
Copy link
Copy Markdown
Member

larsoner commented Jul 9, 2020

(This was discussed in the original PR quite a bit, too)

@hoechenberger
Copy link
Copy Markdown
Member Author

hoechenberger commented Jul 9, 2020 •
edited
Loading

-1 on this. There are lots of options for filtering. I don't see why we want to add this here

I do see your point, but I was feeling that we should provide a sane default here. Less typing and therefore fewer bugs in user code :) Similarly, compute_proj_ecg() already offers "built-in" filtering, so I thought it would make sense to add this here too, for the sake of consistency. For advanced users (and use cases :)) where one needs more sophisticated filters, one can still manually filter and set h_freq=None and work with the preconditioned data.

(This was discussed in the original PR quite a bit, too)

I will read up on this, thanks for the pointer.

@larsoner
Copy link
Copy Markdown
Member

larsoner commented Jul 9, 2020

Similarly, compute_proj_ecg() already offers "built-in" filtering, so I thought it would make sense to add this here too, for the sake of consistency

I actually consider that a kind of a deficiency in the design of compute_proj_ecg. It supports some but not all filtering options and the API is both bloated and incomplete because of this. But also, to at least somewhat justify it, we do filter there by default, I don't think we should for find_bad_channels_maxwell (even though we recommend doing it) because it is a bit sensitive to the parameters you choose.

One thing that might be okay is actually to use **filt_kwargs here. If it's empty, don't filter. If it's not, pass all of them to raw = raw.copy().filter(...). Then you at least implicitly get all options. But really I'm still -0.5 since all we're really saving is a few characters:

raw = raw.copy().filter(2.)
find_bad_channels_maxwell(raw)

# vs
find_bad_channels_maxwell(raw, h_freq=2)

@agramfort
Copy link
Copy Markdown
Member

agramfort commented Jul 9, 2020

what convinces me is that it promotes best practices. If we think that by default users should filter below 40Hz it's a way to help users.

ideally we should filter automatically with the best options to detect artifacts ...

@larsoner
Copy link
Copy Markdown
Member

larsoner commented Jul 9, 2020

Makes sense. And also I was thinking about high-pass filtering, which can really mess with the performance. The low-pass characteristics I think it's less sensitive to. So I can live with this

@agramfort
Copy link
Copy Markdown
Member

agramfort commented Jul 9, 2020 via email

@larsoner
Copy link
Copy Markdown
Member

larsoner commented Jul 9, 2020

No I don't high-pass, I just filter_chpi. MaxFilter does some high-passing but it didn't seem to help much in my testing, and was a big source of variability (between MaxFilter and MNE matching). I struggled with trying to make them equivalent for quite some time and couldn't, hence why I was "stuck" on the idea that this was about high-passing...

@agramfort
Copy link
Copy Markdown
Member

agramfort commented Jul 9, 2020 via email

larsoner
larsoner reviewed Jul 9, 2020
View reviewed changes
mne/preprocessing/maxwell.py
msg = (f'The input data has already been low-pass filtered with a '
f'{raw.info["lowpass"]} Hz cutoff frequency, which is '
f'below the requested cutoff of {h_freq} Hz. Not applying '
f'low-pass filter.')
Copy link
Copy Markdown
Member

@larsoner larsoner Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this to be true you need to then set h_freq = None

Copy link
Copy Markdown
Member

@larsoner larsoner Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh wait nevermind, you don't need to because you actually just filter below!

@larsoner larsoner merged commit 31f4923 into mne-tools:master Jul 9, 2020
@larsoner
Copy link
Copy Markdown
Member

larsoner commented Jul 9, 2020

Thanks @hoechenberger

@hoechenberger
Copy link
Copy Markdown
Member Author

hoechenberger commented Jul 9, 2020

Great, thanks @larsoner

@hoechenberger hoechenberger deleted the find-bads-filter branch July 9, 2020 17:18
hoechenberger added a commit to hoechenberger/mne-bids-pipeline that referenced this pull request Jul 9, 2020
@hoechenberger
No need to low-pass filter manually
2bf6484 
`find_bad_channels_maxwell()` now automatically
low-pass filters by default, thanks to
mne-tools/mne-python#7983
@hoechenberger hoechenberger mentioned this pull request Jul 9, 2020
Merged
agramfort pushed a commit to mne-tools/mne-bids-pipeline that referenced this pull request Jul 9, 2020
@hoechenberger
No need to low-pass filter manually (#153)
efa5700 
`find_bad_channels_maxwell()` now automatically
low-pass filters by default, thanks to
mne-tools/mne-python#7983
larsoner added a commit to larsoner/mne-python that referenced this pull request Jul 14, 2020
@larsoner
Merge remote-tracking branch 'upstream/master' into refactor
c1f79f7 
* upstream/master: (21 commits)
  MRG: Add SSP projectors to Report (mne-tools#7991)
  BUG: Fix warning (mne-tools#8006)
  WIP: TFR Doc/test changes (mne-tools#7998)
  MAINT: Remove numpydoc test on 3.6 (mne-tools#8005)
  MAINT: Better error message for mismatch (mne-tools#8007)
  MRG: Allow removal of active projectors if channels they applied to have meanwhile been dropped (mne-tools#8003)
  MRG Freesurfer coordinate frame tutorial (mne-tools#7578)
  FIX: Fix stockwell checks (mne-tools#7996)
  MRG, ENH: Add array-spacing plugin and reorganize deps (mne-tools#7997)
  MRG, ENH: Reduce memory usage of Welch PSD (mne-tools#7994)
  STY: One more [ci skip]
  STY: Docstyle [ci skip]
  Report parsing (mne-tools#7990)
  MRG, ENH: BrainVision impedance parsing (mne-tools#7974)
  BUG: Fix missing source space points (mne-tools#7988)
  [MRG] Strip base directory name from Report captions when using parse_folder (mne-tools#7986)
  DOC: Update estimates [skip travis] (mne-tools#7987)
  DOC: Try to improve find_bad_channels_maxwell doc (mne-tools#7982)
  VIZ, BUG: fix tickmarks in evoked topomap colorbar (mne-tools#7980)
  Add low-pass filter to find_bad_channels_maxwell() (mne-tools#7983)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@larsoner larsoner larsoner left review comments

@agramfort agramfort agramfort approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hoechenberger @larsoner @agramfort