search

mne-tools / mne-python

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

RuntimeWarning for channels with different filter settings on import #12643

Open cbrnr opened 4 months ago

cbrnr commented 4 months ago

Recently (as of #12441), MNE started to raise warnings when importing EDF/BDF files containing channels with different filter settings (see here):

RuntimeWarning: Channels contain different highpass filters. Highest filter setting will be stored.
RuntimeWarning: Channels contain different lowpass filters. Lowest filter setting will be stored.

There are three issues with this behavior:

  1. All channels are used to check for identical filter settings. This is not a good idea, since different channel types are likely to have different filter settings. In particular, STIM channels are also considered in this comparison, so BDF files always trigger the warning (STIM channels contain empty filter strings).
  2. I think the warning message (and possibly also the resulting behavior) is incorrect. IMO, if channels (of the same type) contain different highpass filters, the lowest filter setting should be stored. Similarly, for different lowpass filters, the highest setting should be stored. These correspond to the weakest thresholds, and this is how it's done in read_raw_brainvision() (see here and here). Interestingly, read_raw_nsx() also seems to do the wrong thing (see here).
  3. In my opinion, raising a RuntimeWarning is too much. Even if the check is only performed within (certain) channel types, I would only log to info.
larsoner commented 4 months ago

It looks like those warnings have been in place for 9 years now. Rather than change their behavior to info now, I'd rather fix (1) and (2). To suppress the warnings (in the meantime or later) you could for example also read with verbose="error".

cbrnr commented 4 months ago

They were in place for so long without being noticed most likely. I've never encountered a BrainVision file that triggered the warning, and the other format is very niche.

I still think different filter settings do not warrant RuntimeWarnings. Why do you think this is so important that an info message is not sufficient? In other words, do you have a reason other than the age?

cbrnr commented 4 months ago

And for EDF/BDF these warnings are new, so if you don't want to change the warnings for the other formats, then let's either do it just for EDF/BDF or revert that change entirely.

larsoner commented 4 months ago

They were in place for so long without being noticed most likely.

I seem to recall there being other PRs that were similar, and IIRC there have been some tests in place for them. But I'd need to look.

Why do you think this is so important that an info message is not sufficient? In other words, do you have a reason other than the age?

Well one thing to keep in mind is that age is at least some justification -- it suggests that behaviors have been around a while with expectations and experiences suggesting that they're somewhere between tolerable and useful. But beyond that, in general when we detect something is wrong/inconsistent/un-representable-in-MNE for a file that could lead to some bad behavior, emitting a warning is a reasonable thing to do, as it's much more visible than info. In the case of wrong/incompatible filter settings (e.g., you think your lowpass for all channels in X when really it's Y for some of them) we tend to emit warnings as you point out above. I think this PR from a few years ago that adds a warning about filtering has a similar idea behind it as well:

https://github.com/mne-tools/mne-python/pull/8584/files#diff-1d508c4fe7f39a5d09e8cd44e2996647850b3398a3813e37e9e97654581b689cR477

So to me the right thing to do here is improve the specificity of the warning as in (1) and (2).

cbrnr commented 4 months ago

I think there's a big difference between incompatible/wrong filter settings vs. just different filter settings. What exactly are the consequences of not knowing that a common cutoff frequency has been set that is compatible with all filter settings? The signals are not affected at all, it's just some meta info field that MNE is setting. And people would still be able to see it in info, which is the default logging level.

But okay, since it is not likely that I'll convince you, someone should take care of (1) and (2). I don't have the time, so if no one can do it soonish I'd still revert the EDF/BDF change as an intermediate fix. Right now, this warning is triggered for all BDF files.

hoechenberger commented 4 months ago

@larsoner My limited understanding is that this warning will be emitted when loading any BDF file, effectively rendering the informative value of the warning useless...

larsoner commented 4 months ago

Yeah if that's the case then that's not good. I'm suggesting that we should make it useful rather than eliminate it in all cases though.

hoechenberger commented 4 months ago

This sounds reasonable to me!!

cbrnr commented 4 months ago

I was trying to argue for a quick hotfix (possibly to be released in 1.7.1), because I currently don't have the time to look into these two issues. I can only revert the warning for EDF/BDF, but if you would like to have a proper fix (i.e. make the warning actually useful and also do the right thing), then someone else would have to implement it in the near future. Again, this warning is triggered for every BDF file no matter what.

Edit: OK, maybe I was wrong. Stim channels already seem to be excluded, and currently I don't know how to reproduce the warning with my BDF files. I'm investigating, but it seems like the warning might not get triggered for any BDF file after all, so this won't be that urgent. I'll report back.

cbrnr commented 4 months ago

I can reproduce it, it is an interaction with the exclude parameter. I'll try to figure out what's wrong.

cbrnr commented 4 months ago

Since the warning has been fixed, there are two remaining issues:

  1. Should we check for identical filter setting within channel types? Currently, all channels (except STIM channels) are used. I'd say YAGNI for now (because we could assume the majority of EDF/BDF files contain just one channel type).
  2. What happens if the channels are associated with different filter settings? The behavior of read_raw_brainvision() is inconsistent with read_raw_{e|b}df() and read_raw_nsx(). Whereas the former uses the lowest (highest) cutoff frequency for different highpass (lowpass) filters, the latter use the highest (lowest) cutoff frequencies for different highpass (lowpass) filters. What is the correct approach here?
larsoner commented 3 months ago

Should we check for identical filter setting within channel types?

I think we might for other readers but no strong feeling

What happens if the channels are associated with different filter settings?

I would go with whichever one we had the most extensive discussions about. Not sure offhand which one that is, will require some GitHub archaeology