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

[qian-chu]

When cropping the start of a recording, raw.first_time is updated while annotations.onset is conveniently untouched. However, when exporting to another format where times are reset (starting from zero), annotations.onset should be corrected so that they represent relative time from the first sample.

This correction has been performed when fmt=‘brainvision’:

https://github.com/mne-tools/mne-python/blob/e2c80104e2d27a9fbd3cb628b13c047ef6fe1ae8/mne/export/_brainvision.py#L78-L85

But is curiously missing when fmt=‘edf’ or fmt=‘eeglab’: https://github.com/mne-tools/mne-python/blob/e2c80104e2d27a9fbd3cb628b13c047ef6fe1ae8/mne/export/_edf.py#L200-L213 https://github.com/mne-tools/mne-python/blob/e2c80104e2d27a9fbd3cb628b13c047ef6fe1ae8/mne/export/_eeglab.py#L28-L32

This PR aims to fix this by performing the similar correction (annotations.onset - raw.first_time

[cbrnr]

When cropping the start of a recording, raw.first_time is corrected while annotations.onset is conveniently untouched.

Hmmm, really? Annotations are not affected by cropping? Why would that be convenient?

[qian-chu]

When cropping the start of a recording, raw.first_time is corrected while annotations.onset is conveniently untouched.

Hmmm, really? Annotations are not affected by cropping? Why would that be convenient?

At least when annotations.orig_time is not None: https://github.com/mne-tools/mne-python/blob/e2c80104e2d27a9fbd3cb628b13c047ef6fe1ae8/mne/io/base.py#L1560-L1570 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 annotations.orig_time. The onset correction should have been corrected when annotations.orig_time is None. Will do a fix later.

[cbrnr]

To be honest, I didn't even know that annotations work like that. I always thought that annotations.onset are onsets in seconds relative to the start of the data. So I expected that cropping should shift the onsets in general, not just for export. If that was the case, we would not have to deal with correcting onsets on export in the first place.

[qian-chu]

To be honest, I didn't even know that annotations work like that. I always thought that annotations.onset are onsets in seconds relative to the start of the data. So I expected that cropping should shift the onsets in general, not just for export. If that was the case, we would not have to deal with correcting onsets on export in the first place.

I'm not too sure how that will work out. Do you mean that cropping should always reset annotations.onset and then force annotations.orig_time=None?

----------- meas_date=XX, orig_time=YY -----------------------------

     |              +------------------+
     |______________|     RAW          |
     |              |                  |
     |              +------------------+
 meas_date      first_samp
     .
     .         |         +------+
     .         |_________| ANOT |
     .         |         |      |
     .         |         +------+
     .     orig_time   onset[0]
     .
     |                   +------+
     |___________________|      |
     |                   |      |
     |                   +------+
 orig_time            onset[0]'

----------- meas_date=XX, orig_time=None ---------------------------

     |              +------------------+
     |______________|     RAW          |
     |              |                  |
     |              +------------------+
     .              N         +------+
     .              o_________| ANOT |
     .              n         |      |
     .              e         +------+
     .
     |                        +------+
     |________________________|      |
     |                        |      |
     |                        +------+
 orig_time                 onset[0]'

----------- meas_date=None, orig_time=YY ---------------------------

     N              +------------------+
     o______________|     RAW          |
     n              |                  |
     e              +------------------+
               |         +------+
               |_________| ANOT |
               |         |      |
               |         +------+

            [[[ CRASH ]]]

----------- meas_date=None, orig_time=None -------------------------

     N              +------------------+
     o______________|     RAW          |
     n              |                  |
     e              +------------------+
     .              N         +------+
     .              o_________| ANOT |
     .              n         |      |
     .              e         +------+
     .
     N                        +------+
     o________________________|      |
     n                        |      |
     e                        +------+
 orig_time                 onset[0]'

[cbrnr]

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.

[hoechenberger]

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 😅

[qian-chu]

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.

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 meas_date=None, orig_time=YY it should result in error, yet in crop() it asserts the following: https://github.com/mne-tools/mne-python/blob/4954672afd85a8f01acf2c75f015ecadf0a53c59/mne/io/base.py#L1562-L1563 instead of

if self.info["meas_date"] is None:
     assert annotations.orig_time is None

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.

[qian-chu]

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 main of MNE

Original raw array

BrainVision (functions properly)

EDF

EEGLAB

Outputs using the PR branch

Original raw array

BrainVision

EDF

EEGLAB

[cbrnr]

Very nice, thanks for this example, this makes the issue really easy to see! Could you take a look at the failing tests?

[cbrnr]

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 _sync_onset? I think it would be great to be as consistent as possible.

For merging, I'd appreciate if @larsoner and/or @drammock could have a final look.

[qian-chu]

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 _sync_onset? I think it would be great to be as consistent as possible.

For merging, I'd appreciate if @larsoner and/or @drammock could have a final look.

It might be a bit tricky as _mne_annots2pybv_events() loops over annotations instead of taking the numpy arrays individually:

https://github.com/mne-tools/mne-python/blob/fd8c1eed5209393e6a2527342b61354efcb813ee/mne/export/_brainvision.py#L108-L116

One would need to change the code structure if _sync_onset() is to be used. However, we can simply add assert raw.info["meas_date"] == raw.annotations.orig_time to impose the same requirement as on other formats. What do you think?

Also I can add a note to %(export_warning_note_raw)s to document the new requirement.

[cbrnr]

OK, if it's too complicated then don't bother trying to use _sync_onset(). But yes, good idea to add an assert and documentation (and maybe a comment explaining why we're not using _sync_onset() here)!

[qian-chu]

Hi everyone, is there anything else needed for this PR to be completed? Thanks!

[cbrnr]

I think this is good to go, but can you rebase please so that all tests pass?

[qian-chu]

Oops, seems I've triggered a grand review request through a rookie rebase... Sorry for bugging everyone

[cbrnr]

Thanks @qian-chu, great contribution!

[qian-chu]

Thanks everyone!