[BUG] Correct annotation onset for exportation to EDF and EEGLAB#12656
[BUG] Correct annotation onset for exportation to EDF and EEGLAB#12656cbrnr merged 65 commits intomne-tools:mainfrom
Conversation
| @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 | ||
| ) |
There was a problem hiding this comment.
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.
mne/export/tests/test_export.py
Outdated
| if tmin % 1 == 0: | ||
| expectation = nullcontext() | ||
| else: | ||
| expectation = pytest.warns( | ||
| RuntimeWarning, match="EDF format requires equal-length data blocks" | ||
| ) |
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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.
Hmmm, really? Annotations are not affected by cropping? Why would that be convenient? |
At least when Lines 1560 to 1570 in e2c8010 So that when annotations have their own time reference, cropping the data wouldn't affect them. Actually this is a good reminder that we might need to account for different |
|
To be honest, I didn't even know that annotations work like that. I always thought that |
I'm not too sure how that will work out. Do you mean that cropping should always reset |
|
Maybe it's just me, but I gave up trying to understand how this works. The ASCII diagram is probably meant to be helpful, but for me it is the complete opposite, I have no idea how these different concepts (meas_date, orig_time, first_samp, and whatnot) actually work, sorry. |
|
I agree, I've tried several times over the past couple of years to decipher what it's trying to tell me and at one point just gave up. It's just been trial and error for me regarding all things annotations ever since 😅 |
It's definitely OK! As I'm re-looking at this PR after some time I'm also struggling to wrap my head around this system. FYI this diagram was copied from https://mne.tools/dev/generated/mne.Annotations.html. One potential conflict I found is, the diagram says when Lines 1562 to 1563 in 4954672 instead of If someone who's familiar with the design can clarify that would be great. But I do confirm that the EDF and EEGLAB export will malfunction without correcting for first_time so eventually we would want this fix. |
|
Coming back to this issue after some time, I re-confirmed the existence of the problem with a minimalist code: import numpy as np
from mne import create_info, Annotations
from mne.io import RawArray, read_raw_brainvision, read_raw_edf, read_raw_eeglab
from mne.viz import set_browser_backend
set_browser_backend('qt')
# Create a raw object of SR 1000 Hz, all zero, except for 1s of 1e-6 from 2-3s
data = np.zeros((1, 5000))
data[0, 2000:3000] = 1
scalings = dict(eeg=1)
info = create_info(['CH1'], 1000, ['eeg'])
raw_orig = RawArray(data, info)
annot = Annotations(onset=[2], duration=[1], description=['stim'])
raw_orig.set_annotations(annot)
# Crop raw to 1-5s
raw_orig.crop(1)
fig_orig = raw_orig.plot(scalings=scalings)
fig_orig.grab().save('orig.png')
# Export to BrainVision and re-read
raw_orig.export('test.vhdr')
raw_brainvision = read_raw_brainvision('test.vhdr')
fig_brainvision = raw_brainvision.plot(scalings=scalings)
fig_brainvision.grab().save('brainvision.png')
# Export to EDF and re-read
raw_orig.export('test.edf')
raw_edf = read_raw_edf('test.edf')
fig_edf = raw_edf.plot(scalings=scalings)
fig_edf.grab().save('edf.png')
# Export to EEGLAB and re-read
raw_orig.export('test.set')
raw_eeglab = read_raw_eeglab('test.set')
fig_eeglab = raw_eeglab.plot(scalings=scalings)
fig_eeglab.grab().save('eeglab.png')Outputs using the current
|
|
Very nice, thanks for this example, this makes the issue really easy to see! Could you take a look at the failing tests? |
|
This looks good to me, just one last comment: do you think it makes sense to update the export code for BrainVision so that all formats consistently use For merging, I'd appreciate if @larsoner and/or @drammock could have a final look. |
It might be a bit tricky as mne-python/mne/export/_brainvision.py Lines 108 to 116 in fd8c1ee One would need to change the code structure if Also I can add a note to |
|
OK, if it's too complicated then don't bother trying to use |
Head branch was pushed to by a user without write access
mne/utils/docs.py
Outdated
| docdict["export_warning_annotations_raw"] = """\ | ||
| .. warning:: | ||
| When exporting ``Raw`` with annotations, ``raw.info["meas_date"]`` must be the same | ||
| as ``raw.annotations.orig_time``. This guarantees that the annotations are in the | ||
| same reference frame as the samples. | ||
| When `Raw.first_time` is not zero (e.g., after cropping), the onsets are | ||
| automatically corrected so that onsets are always relative to the first sample. | ||
| """ | ||
|
|
There was a problem hiding this comment.
I do not think it is necessary to add a docdict entry if this entry is only used exactly once. Please move that text directly into the docstring.
There was a problem hiding this comment.
Just did. Let me know how it looks to you!
|
Hi everyone, is there anything else needed for this PR to be completed? Thanks! |
|
I think this is good to go, but can you rebase please so that all tests pass? |
…12828) Co-authored-by: shristi <shristi.baral@aalto.fi> Co-authored-by: Eric Larson <larson.eric.d@gmail.com> Co-authored-by: Daniel McCloy <dan@mccloy.info>
Co-authored-by: Daniel McCloy <dan@mccloy.info> Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
…ort `Spectrum` data (mne-tools#13058) Co-authored-by: Daniel McCloy <dan@mccloy.info>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
…s#13069) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
…-tools#13067) Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
… Principal Component Analysis - Optimal Basis Sets (PCA-OBS) algorithm (mne-tools#13037) Co-authored-by: Emma Bailey <bailey@cbs.mpg.de> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Steinn Magnusson <s.magnusson@senec.com> Co-authored-by: Eric Larson <larson.eric.d@gmail.com> Co-authored-by: emma-bailey <93327939+emma-bailey@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Daniel McCloy <dan@mccloy.info>
f2381db to
96fb55d
Compare
|
Oops, seems I've triggered a grand review request through a rookie rebase... Sorry for bugging everyone |
|
Thanks @qian-chu, great contribution! |
|
Thanks everyone! |
* upstream/main: (57 commits) Allow lasso selection sensors in a plot_evoked_topo (mne-tools#12071) MAINT: Fix doc build (mne-tools#13076) BUG: Improve sklearn compliance (mne-tools#13065) [pre-commit.ci] pre-commit autoupdate (mne-tools#13073) MAINT: Add Numba to 3.13 test (mne-tools#13075) Bump autofix-ci/action from ff86a557419858bb967097bfc916833f5647fa8c to 551dded8c6cc8a1054039c8bc0b8b48c51dfc6ef in the actions group (mne-tools#13071) [BUG] Correct annotation onset for exportation to EDF and EEGLAB (mne-tools#12656) New feature for removing heart artifacts from EEG or ESG data using a Principal Component Analysis - Optimal Basis Sets (PCA-OBS) algorithm (mne-tools#13037) [BUG] Fix taper weighting in computation of TFR multitaper power (mne-tools#13067) [FIX] Reading an EDF with preload=False and mixed frequency (mne-tools#13069) Fix evoked topomap colorbars, closes mne-tools#13050 (mne-tools#13063) [pre-commit.ci] pre-commit autoupdate (mne-tools#13060) BUG: Fix bug with interval calculation (mne-tools#13062) [DOC] extend documentation for add_channels (mne-tools#13051) Add `combine_tfr` to API (mne-tools#13054) Add `combine_spectrum()` function and allow `grand_average()` to support `Spectrum` data (mne-tools#13058) BUG: Fix bug with helium anon (mne-tools#13056) [ENH] Add option to store and return TFR taper weights (mne-tools#12910) BUG: viz plot window's 'title' argument showed no effect. (mne-tools#12828) MAINT: Ensure limited set of tests are skipped (mne-tools#13053) ...








When cropping the start of a recording,
raw.first_timeis updated whileannotations.onsetis conveniently untouched. However, when exporting to another format where times are reset (starting from zero),annotations.onsetshould be corrected so that they represent relative time from the first sample.This correction has been performed when
fmt=‘brainvision’:mne-python/mne/export/_brainvision.py
Lines 78 to 85 in e2c8010
But is curiously missing when
fmt=‘edf’orfmt=‘eeglab’:mne-python/mne/export/_edf.py
Lines 200 to 213 in e2c8010
mne-python/mne/export/_eeglab.py
Lines 28 to 32 in e2c8010
This PR aims to fix this by performing the similar correction (
annotations.onset - raw.first_time