search

mne-tools / mne-python

MNE: Magnetoencephalography (MEG) and Electroencephalography (EEG) in Python
https://mne.tools
BSD 3-Clause "New" or "Revised" License
2.71k stars 1.32k forks source link

BrainVision reader treatment of info['lowpass'] #3577

Closed larsoner closed 8 years ago

larsoner commented 8 years ago

Currently the BV reader sets info['lowpass'] to the lowest filter that was applied to any channel. I think this should be changed due to:

  1. Inconsistent with how we treat this field in raw.filter(..., picks=picks), which only sets it if all data channels are filtered.
  2. Dangers during decimation. Users can easily assume data are sufficiently low-passed when they are not, and get aliasing during epoching.
  3. Dangers during processing. Users can have artifacts they think they have suppressed (e.g., line noise) when they have not.

Advantages of the existing approach (supported by @jona-sassenhagen and @palday):

  1. It is potentially more conservative from a filtering-induced artifact approach, at least for info['highpass'].
  2. It is dangerous to say e.g. info['lowpass'] = sfreq/2. when some of the channels have actually been filtered, as users may think they can analyze data in regions that have been suppressed.

@agramfort @dengemann thoughts on this one?

agramfort commented 8 years ago

+1

palday commented 8 years ago

I can implement this (it is a trivial change from a code standpoint), but a few questions for further thought:

  1. Should we implement some method / have an optional return value from the BV reader with a list of channels and filter settings so that the user can see what the original values were?
  2. Does it make sense to have an API change to support the old behavior? Something like heterogeneous_filter='strongest'|'weakest' with the default in 0.13 being strongest and changing to weakest in 0.14 (much like the recent changes to the FIR filter settings)?
agramfort commented 8 years ago

to me it's a bug and I would not change API for this.

it's a bug fix.

agramfort commented 8 years ago

can this be closed?

palday commented 8 years ago

Close it, we can finish off any remaining bits in the PR (#3593).