fix prefilter management for EDF/BDF by rcmdnk · Pull Request #12441 · mne-tools/mne-python

rcmdnk · 2024-02-14T12:39:36Z

What does this implement/fix?

For EDF, BDF, and GDF formats, prefilter information, including highpass and lowpass filters, is stored in the info structure.
Arrays for highpass/lowpass are generated by the _parse_prefilter_string function, which omits adding a value if no filter information is found.

These arrays are constructed using all channels except the last one, which is presumed to be an Annotation or TAL channel.

I think it should use only selected channels.

Furthermore, it is occasionally necessary to access highpass/lowpass values in _raw_extras for corresponding channel information. To address this, I suggest the following modifications:

The method to create prefiltering values is now adjusted to include only selected channels, using [sel].
The _parse_prefilter_string function has been improved to insert an empty string "" when no information is available. It has also been enhanced to parse strings like "DC".

I have reviewed filter string formats for
Biosemi EEG ECG EMG BSPM NEURO amplifiers systems, EDF specification, EDF+ specification.

In _get_info, where highpass/lowpass values are assigned to info, the current code exhibits bugs:

The condition all(highpass) is always true since highpass and lowpass are arrays of non-empty strings. The correct approach to check if all values are the same should be like np.all(highpass == highpass[0]).
The assignment info["highpass"] = float(np.max(highpass)) fails because highpass contains strings. However, due to the always-true nature of all(highpass), this code is never executed.

This PR addresses and fixes these issues as well. With these changes, some EDF files will trigger warnings about varying highpass and lowpass filter settings across channels, which, due to a bug, were previously not displayed. These warnings are essential for ensuring data consistency and integrity.

larsoner

Thanks for the explanation, sounds reasonable and well thought out to me!

CIs fail because now more warnings need to be caught, see:

https://github.com/mne-tools/mne-python/actions/runs/7901365815/job/21564802456?pr=12441#step:17:4232

Locally if you make sure you have mne-testing-data and pytest>=8 installed you can do pytest mne/io/edf for example, then you should be able to replicate the failures rather than wait for CIs to complain

larsoner · 2024-02-14T14:38:49Z

mne/io/edf/edf.py

-            pass
-        elif highpass[0] == "DC":
-            info["highpass"] = 0.0
+    if "highpass" in edf_info and len(edf_info["highpass"]):


More compact equivalent if you want it

Suggested change

if "highpass" in edf_info and len(edf_info["highpass"]):

if len(edf_info.get("highpass", [])):

Thanks for the tips.
I updated these codes for GDF with _set_prefilter function
and similar code was implemented there.

rcmdnk · 2024-02-14T15:46:49Z

@larsoner
Thanks for the reply.
Yes, it seems several tasks need to be updated which I did not checked locally.
I will update with local tests for these jobs.

rcmdnk · 2024-02-15T10:12:18Z

mne/io/edf/tests/test_edf.py

-        and len(out["highpass"])
-        and out["highpass"][0] == "0.0"
-    ):
-        out["highpass"][0] = "10.0"


String "0.000" for lowpass was converted into float 0 in the previous code.
(it does not pass if lowpass[0] in ("NaN", "0", "0.0"):)
In this case, both lowpass and highpass is set to 0, and
if info["highpass"] > info["lowpass"]:
is False.

I think this is a kind of special test for highpass > lowpass case.

With the new code, "0.000" for lowpass is skipped to be set as info["lowpass"] and it remains as default (sfreq/2).
Therefore, lowpass=256, highpass=0 are correctly obtained.

To make highpass > lowpass case, _hp_lp_mod function was added to force to set highpass > lowpass.

Related PR: #8584

larsoner

Looks reasonable to me. @cbrnr looks like you've worked on this stuff a bit, can you look?

cbrnr · 2024-02-16T14:11:22Z

Yes, I can take a look early next week!

mne/report/tests/test_report.py

Fix prefilter management for GDF (highpass/lowpass are float instead of list of str) Fix tests which throw warnings of "Channels contain different..."

…highpass/lowpass of ndarray of str to _get_info. gdf returns highpass/lowpass of ndarray of np.float32 to _get_info)

… in _raw_extras Select channels of sel but stim chanenl to set highpass/lowpass in info

larsoner · 2024-04-15T16:25:50Z

@cbrnr you happy with this now? If so we could get it into 1.7 I think

cbrnr · 2024-04-15T16:33:47Z

It's been some time since I commented on this PR. Could you or @rcmdnk summarize the public (user-facing) changes? Are there any? "Fix pre-filtering management" doesn't really tell me a lot about the changes (and this more detailed information should go into the changelog entry as well). Thanks!

rcmdnk · 2024-04-15T17:51:57Z

@cbrnr
The problem is that
the current implementation inaccurately handles the highpass and lowpass prefilter information in the Raw.info.

Bugs:

All prefilter information other than empty channels are stored in _raw_extras
- no selection by selected channels, empty channels are ignored.
all(highpass) seems to be used to check uniformity of highpass values across channels.
- However, it fails to trigger the warning for differing highpass filters because it incorrectly checks for all True values instead of uniformity.
info["highpass"] = float(np.max(highpass)) does not work because highpass is an array of strings. (but this does not occurs due to the previous all check.

Changes Introduced in This PR:

The info['highpass'] and info['lowpass'] will now correctly reflect the maximum and minimum prefilter values from the selected channels, respectively.
Remain all channel values in _raw_extras
- Put empty string "" if no the information is empty.
- Select values for selected channels in _get_info to make highpass/lowpass of Raw.info
Warn if selected channels have different prefiltering values.

cbrnr · 2024-04-16T07:36:49Z

In it goes – the changelog entry is OK, people can always refer to this thread for more details. Thanks @rcmdnk!

* upstream/main: fix prefilter management for EDF/BDF (mne-tools#12441) [pre-commit.ci] pre-commit autoupdate (mne-tools#12541) ENH: Allow removal of (up to two) bad marker coils in read_raw_kit() (mne-tools#12394) Implement `picks` argument to `Raw.plot()` (mne-tools#12467) Add Meggie under Related Software documentation (mne-tools#12540)

larsoner reviewed Feb 14, 2024

View reviewed changes

rcmdnk force-pushed the fix_prefilter branch 2 times, most recently from 79131c2 to bced368 Compare February 15, 2024 07:14

rcmdnk marked this pull request as ready for review February 15, 2024 09:58

rcmdnk requested review from agramfort, dengemann, drammock, hoechenberger and jasmainak as code owners February 15, 2024 09:58

rcmdnk commented Feb 15, 2024

View reviewed changes

larsoner approved these changes Feb 16, 2024

View reviewed changes

cbrnr reviewed Feb 19, 2024

View reviewed changes

mne/report/tests/test_report.py Outdated Show resolved Hide resolved

rcmdnk force-pushed the fix_prefilter branch from 6fa42b7 to 0f2f7c2 Compare February 19, 2024 18:54

rcmdnk added 6 commits February 20, 2024 05:34

fix prefilter management for EDF/BDF

43a1d21

add 12441.bugfix.rst

cc73348

Add _set_prefilter method in mne.io.edf.edf to simplify filter settings

353d0c1

Fix prefilter management for GDF (highpass/lowpass are float instead of list of str) Fix tests which throw warnings of "Channels contain different..."

Add test case to warn "greater than lowpass..."

a067d6a

support only list (or np.ndarray) in _set_prefilter (edf, bdf return …

5890aba

…highpass/lowpass of ndarray of str to _get_info. gdf returns highpass/lowpass of ndarray of np.float32 to _get_info)

Parse all prefiltering of edf/bdf in _read_edf_header and remain them…

2b473d6

… in _raw_extras Select channels of sel but stim chanenl to set highpass/lowpass in info

rcmdnk force-pushed the fix_prefilter branch from 0f2f7c2 to 2b473d6 Compare February 19, 2024 20:34

rcmdnk and others added 2 commits February 20, 2024 06:13

fix: Handle missing 'highpass' and 'lowpass' keys safely in EDF tests

d787573

Merge branch 'main' into fix_prefilter

424e85d

larsoner added this to the 1.8 milestone Apr 15, 2024

cbrnr merged commit 73661d1 into mne-tools:main Apr 16, 2024

rcmdnk deleted the fix_prefilter branch April 25, 2024 06:30

cbrnr mentioned this pull request Jun 4, 2024

RuntimeWarning for channels with different filter settings on import #12643

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix prefilter management for EDF/BDF#12441

fix prefilter management for EDF/BDF#12441
cbrnr merged 8 commits intomne-tools:mainfrom
rcmdnk:fix_prefilter

rcmdnk commented Feb 14, 2024

Uh oh!

larsoner left a comment

Uh oh!

larsoner Feb 14, 2024

Uh oh!

rcmdnk Feb 15, 2024

Uh oh!

rcmdnk commented Feb 14, 2024

Uh oh!

rcmdnk Feb 15, 2024 •

edited

Loading

Uh oh!

larsoner left a comment

Uh oh!

cbrnr commented Feb 16, 2024

Uh oh!

Uh oh!

larsoner commented Apr 15, 2024

Uh oh!

cbrnr commented Apr 15, 2024

Uh oh!

rcmdnk commented Apr 15, 2024 •

edited

Loading

Uh oh!

cbrnr commented Apr 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	if "highpass" in edf_info and len(edf_info["highpass"]):
	if len(edf_info.get("highpass", [])):

Uh oh!

Conversation

rcmdnk commented Feb 14, 2024

What does this implement/fix?

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

larsoner Feb 14, 2024

Choose a reason for hiding this comment

Uh oh!

rcmdnk Feb 15, 2024

Choose a reason for hiding this comment

Uh oh!

rcmdnk commented Feb 14, 2024

Uh oh!

rcmdnk Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

cbrnr commented Feb 16, 2024

Uh oh!

Uh oh!

larsoner commented Apr 15, 2024

Uh oh!

cbrnr commented Apr 15, 2024

Uh oh!

rcmdnk commented Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbrnr commented Apr 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rcmdnk Feb 15, 2024 •

edited

Loading

rcmdnk commented Apr 15, 2024 •

edited

Loading