Skip to content
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

Add low-pass filter to find_bad_channels_maxwell() #7983

Merged
merged 5 commits into from
Jul 9, 2020

Conversation

hoechenberger
Copy link
Member

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.

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.
@@ -1975,6 +1977,18 @@ def find_bad_channels_maxwell(

.. versionadded:: 0.20
"""
if h_freq is not None:
if 'lowpass' in raw.info:
Copy link
Member

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
Member Author

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?

Copy link
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
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
Member

larsoner commented Jul 9, 2020

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

@hoechenberger
Copy link
Member Author

hoechenberger 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

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
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
Member

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
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
Member

agramfort commented Jul 9, 2020 via email

@larsoner
Copy link
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
Member

agramfort commented Jul 9, 2020 via email

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
Member

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
Member

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
Member

larsoner commented Jul 9, 2020

Thanks @hoechenberger

@hoechenberger
Copy link
Member Author

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
`find_bad_channels_maxwell()` now automatically
low-pass filters by default, thanks to
mne-tools/mne-python#7983
agramfort pushed a commit to mne-tools/mne-bids-pipeline that referenced this pull request Jul 9, 2020
`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
* 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants