Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[BUG] Correct annotation onset for exportation to EDF and EEGLAB #12656

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

[BUG] Correct annotation onset for exportation to EDF and EEGLAB #12656

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
b2aa09b
correct annotation onset during exportation
qian-chu Jun 10, 2024
1643b3c
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 10, 2024
c837852
Create 12656.bugfix.rst
qian-chu Jun 10, 2024
bbe6501
formatting
qian-chu Jun 10, 2024
1eda973
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 10, 2024
78eb8ac
formatting func desc
qian-chu Jun 10, 2024
590cf16
summary line and desc separation
qian-chu Jun 10, 2024
970303c
fix typo
qian-chu Jun 10, 2024
cc96f10
add period
qian-chu Jun 10, 2024
51b8c1f
Merge branch 'main' into correct_annot_export
qian-chu Jul 3, 2024
bd2fa76
Merge branch 'mne-tools:main' into correct_annot_export
qian-chu Aug 24, 2024
f8f118a
use _sync_onset
qian-chu Aug 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion mne/export/_edf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,9 @@ def _export_raw(fname, raw, physical_range, add_ch_type):
annotations = []
for desc, onset, duration, ch_names in zip(
raw.annotations.description,
raw.annotations.onset,
# subtract raw.first_time because EDF marks events starting from the first
# available data point and ignores raw.first_time
raw.annotations.onset - raw.first_time,
raw.annotations.duration,
raw.annotations.ch_names,
):
Expand Down
4 changes: 3 additions & 1 deletion mne/export/_eeglab.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ def _export_raw(fname, raw):

annotations = [
raw.annotations.description,
raw.annotations.onset,
# subtract raw.first_time because EEGLAB marks events starting from the first
# available data point and ignores raw.first_time
raw.annotations.onset - raw.first_time,
raw.annotations.duration,
]
eeglabio.raw.export_set(
Expand Down
74 changes: 67 additions & 7 deletions mne/export/tests/test_export.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,43 @@ def test_export_raw_eeglab(tmp_path):
raw.export(temp_fname, overwrite=True)


@pytest.mark.parametrize("tmin", (0, 1, 5, 10))
def test_export_raw_eeglab_annotations(tmp_path, tmin):
"""Test that exporting EEGLAB preserves annotations and corects for raw.first_time."""
pytest.importorskip("eeglabio")
raw = read_raw_fif(fname_raw, preload=True)
raw.apply_proj()
annotations = Annotations(
onset=[0.01, 0.05, 0.90, 1.05],
duration=[0, 1, 0, 0],
description=["test1", "test2", "test3", "test4"],
ch_names=[["MEG 0113"], ["MEG 0113", "MEG 0132"], [], ["MEG 0143"]],
)
raw.set_annotations(annotations)
raw.crop(tmin)

# export
temp_fname = tmp_path / "test.set"
raw.export(temp_fname)

# read in the file
with pytest.warns(RuntimeWarning, match="is above the 99th percentile"):
raw_read = read_raw_eeglab(temp_fname, preload=True, montage_units="m")
assert raw_read.first_time == 0

valid_annot = raw.annotations.onset >= tmin
assert_array_almost_equal(
raw.annotations.onset[valid_annot] - raw.first_time,
raw_read.annotations.onset - raw_read.first_time,
)
assert_array_equal(
raw.annotations.duration[valid_annot], raw_read.annotations.duration
)
assert_array_equal(
raw.annotations.description[valid_annot], raw_read.annotations.description
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually should this annotation test for EEGLAB been written before, the bug should be noticable because the test Raw has actually been cropped. The saved onset would not correspond to the original onset.



def _create_raw_for_edf_tests(stim_channel_index=None):
rng = np.random.RandomState(12345)
ch_types = [
Expand Down Expand Up @@ -217,8 +254,9 @@ def test_edf_physical_range(tmp_path):


@edfio_mark()
def test_export_edf_annotations(tmp_path):
"""Test that exporting EDF preserves annotations."""
@pytest.mark.parametrize("tmin", (0, 0.005, 0.03, 1))
def test_export_edf_annotations(tmp_path, tmin):
"""Test that exporting EDF preserves annotations and corects for raw.first_time."""
raw = _create_raw_for_edf_tests()
annotations = Annotations(
onset=[0.01, 0.05, 0.90, 1.05],
Expand All @@ -227,17 +265,39 @@ def test_export_edf_annotations(tmp_path):
ch_names=[["0"], ["0", "1"], [], ["1"]],
)
raw.set_annotations(annotations)
raw.crop(tmin)
assert raw.first_time == tmin

if tmin % 1 == 0:
expectation = nullcontext()
else:
expectation = pytest.warns(
RuntimeWarning, match="EDF format requires equal-length data blocks"
)
Comment on lines +318 to +323
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this check doing? If you are checking for tmin to be an integer, you could also use tmin.is_integer(), but is this what is required to have "equal-length data blocks"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the constructed raw signal is 2 sec long and edfio can segment it into 2 data records of 1 sec. If a non-integer amount of time is cropped, then the signal is no longer a multiple of 1 sec and edfio will append zeroes and issue a RuntimeWarning. Maybe this should have been a test on its own but I'm adding it here since pytest wouldn't pass me otherwise.

As for the %1 == 0 condition, I was thinking to make space for more flexible use should I know how edfio determines data record length. For example if one can specify a data record length of .5 or 2 s, then the statement can be replaced with %data_length == 0. But I agree it looks uncessary in its current form.


# export
temp_fname = tmp_path / "test.edf"
raw.export(temp_fname)
with expectation:
raw.export(temp_fname, physical_range=(0, 10))

# read in the file
raw_read = read_raw_edf(temp_fname, preload=True)
assert_array_equal(raw.annotations.onset, raw_read.annotations.onset)
assert_array_equal(raw.annotations.duration, raw_read.annotations.duration)
assert_array_equal(raw.annotations.description, raw_read.annotations.description)
assert_array_equal(raw.annotations.ch_names, raw_read.annotations.ch_names)
assert raw_read.first_time == 0

valid_annot = raw.annotations.onset >= tmin
assert_array_almost_equal(
raw.annotations.onset[valid_annot] - raw.first_time,
raw_read.annotations.onset - raw_read.first_time,
)
assert_array_equal(
raw.annotations.duration[valid_annot], raw_read.annotations.duration
)
assert_array_equal(
raw.annotations.description[valid_annot], raw_read.annotations.description
)
assert_array_equal(
raw.annotations.ch_names[valid_annot], raw_read.annotations.ch_names
)


@edfio_mark()
Expand Down
Loading