mne-tools / mne-python

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

Bug in IO module for BrainVision: Wrong conversion from Seconds to Hertz #4998

Closed sappelhoff closed 6 years ago

sappelhoff commented 6 years ago

In BrainVision Recorder *.vhdr files, the highpass filter settings are often listed as "low-cutoff in seconds". The relevant mne.io function translates this into Hz as such:

https://github.com/mne-tools/mne-python/blob/239c83dfe0686084d1c96c945579dc8cc10e895c/mne/io/brainvision/brainvision.py#L597-L598

With this method, the standard setting of low-cutoff=10s is translated to highpass=0.1Hz. However, the BrainProducts website specifies 0.016Hz (see under "Lower cutoff frequency (high pass) / time constant").

The conversion from seconds to Hz is probably not implemented correctly in the mne.io function for brainvision. Or am I missing something?

Some references:

sappelhoff commented 6 years ago

The fix seems simple enough:

if hp_s:
    info['highpass'] = 1. / (2 * np.pi * info['highpass'])

... and using this approach also for the remaining lines (lowpass filters, and the case for heterogeneous filters)

larsoner commented 6 years ago

Yes this looks correct to me. I've actually never seen this nomenclature before.

larsoner commented 6 years ago

... although "time constant" rings a bell in terms of analog filtering and circuits. I wonder if that's where it comes from.

sappelhoff commented 6 years ago

I find it unnecessarily confusing and I am wondering about the benefits of specifying 10seconds instead of 0.016Hz ... maybe because it is actually 0.015915494309189534 and then 10seconds is more straight forward and doesn't need rounding? Anyways, I have opened a PR

dmedine commented 6 years ago

Hello. My name is David Medine and I am a developer at Brain Products. The reason they use a time constant to represent the nature of a filter is for strange, convoluted, historical reasons.

What is important is that sappelhoff is correct. The denominator should have a factor of 2pi in it.

larsoner commented 6 years ago

Thanks for the confirmation @dmedine. Looks like #5000 should be correct, then.