Mne-python: BUG: incorrect freq bandwidth in FilterEstimator

[kingjr]

@trachelr I'm not entirely sure but I think there is a problem with the transition bandwidths in your FilterEstimator.

Can you confirm or infirm that the low and high bandwidth are adequate?
https://github.com/mne-tools/mne-python/blob/master/mne/decoding/transformer.py#L484

If not, I'll mark the issue as a bug, but don't worry about submitting a PR soon (although you may), we're about to refactor all decoding transformers and estimators.

If it's working as intended, well, my bad..

[kingjr]

Here's what I would have expected:

kwargs = dict(Fs=self.sfreq, filter_length=self.filter_length,
              method=self.method, iir_params=self.iir_params,
              copy=False, verbose=False, n_jobs=self.n_jobs)

if self.l_freq is None and self.h_freq is not None:
    filter_func = low_pass_filter
    kwargs['Fp'] = self.h_freq
    kwargs['trans_bandwidth'] = self.h_trans_bandwidth

if self.l_freq is not None and self.h_freq is None:
    filter_func = high_pass_filter
    kwargs['Fp'] = self.l_freq
    kwargs['trans_bandwidth'] = self.l_trans_bandwidth

if self.l_freq is not None and self.h_freq is not None:
    filter_func = band_pass_filter
    kwargs['Fp1'] = self.l_freq
    kwargs['Fp2'] = self.h_freq
    kwargs['l_trans_bandwidth'] = self.l_trans_bandwidth
    kwargs['h_trans_bandwidth'] = self.h_trans_bandwidth

return filter_func(epoch_data, **kwargs)

[kingjr]

(BTW @Eric89GXL, would it make sense to change band_pass_filter to handle Fp1=None or Fp2=None ? Else we could add a generic filter function that applies the above. It seems that this kind of sorting should be handled at a pretty low level, not by a decoding transformer)

[jasmainak]

@kingjr while you're at it, keep in mind that some of these things were added with the real-time functionality in mind. They were made to work on Epochs objects for that reason. We should be careful not break it as I'm aware there are quite many people using it ...

[kingjr]

(@jasmainak We should discussed this elsewhere. We've chatted quite a lot with @dengemann and @kaichogami on this topic, and the issue is that

• passing epochs cannot comply with sklearn functions (e.g. cross_val_score, cv etc).
• Some sklearn objects expect 2D arrays (several ensemble objects)

Furthermore there are some inconsistencies across estimators:

• some like PSDEstimator and FilterEstimator take 3D numpy array,
• other like Xdawn and GAT takes mne epochs

So I'm afraid we'll have to break API at some point... and rather sooner than later. )

[jasmainak]

if the real-time examples work without breaking, I'm fine with the changes.

[larsoner]

would it make sense to change band_pass_filter to handle Fp1=None or Fp2=None

No because then it's a misnomer

Else we could add a generic filter function that applies the above.

Yes that sounds better :)

You can probably get most of the logic from mne.io.Raw.filter and move it to a new mne.filter._filter function. For now please make it private so we can tweak it as we want for the upcoming filtering "auto" modes.

[kingjr]

@jasmainak We'll do our best ;) I think one possible solution would be to recognize the input type (epochs, 2D array, 3D array).

@Eric89GXL Ok, I'll open an issue, but I have too many PRs to finish first.

Regarding the present issue, do you agree that the following lines seem weird:

if self.l_freq is None and self.h_freq is not None:
    epochs_data = \
        low_pass_filter(epochs_data, self.info['sfreq'], self.h_freq,
                        filter_length=self.filter_length,
                        trans_bandwidth=self.l_trans_bandwidth,  # XXX <=====
                        method=self.method, iir_params=self.iir_params,
                        picks=self.picks, n_jobs=self.n_jobs,
                        copy=False, verbose=False)

if self.l_freq is not None and self.h_freq is None:
    epochs_data = \
        high_pass_filter(epochs_data, self.info['sfreq'], self.l_freq,
                         filter_length=self.filter_length,
                         trans_bandwidth=self.h_trans_bandwidth, # XXX <=====
                         method=self.method,
                         iir_params=self.iir_params,
                         picks=self.picks, n_jobs=self.n_jobs,
                         copy=False, verbose=False)

if self.l_freq is not None and self.h_freq is not None:
    if self.l_freq < self.h_freq:
        epochs_data = \
            band_pass_filter(epochs_data, self.info['sfreq'],
                             self.l_freq, self.h_freq,
                             filter_length=self.filter_length,
                             l_trans_bandwidth=self.l_trans_bandwidth,  # XXX <=====
                             h_trans_bandwidth=self.h_trans_bandwidth,  # XXX <=====
                             method=self.method,
                             iir_params=self.iir_params,
                             picks=self.picks, n_jobs=self.n_jobs,
                             copy=False, verbose=False)

[larsoner]

It should match how it is done in raw.filter, so it looks like the low-pass and high-pass conditions are wrong

[trachelr]

Hi guys, sorry for the delay... I'm little bit out of focus.
@kingjr @Eric89GXL just open a PR and I'll fix it

[jasmainak]

@kingjr maybe this could be combined with #3395 and fixed in a single PR

[larsoner]

I think this has been fixed, but feel free to reopen if I'm mistaken