MRG: Enforce specification of all annotation descriptions in event_id if event_id is passed to write_raw_bids()

[hoechenberger]

Fixes #1084

Merge checklist

Maintainer, please confirm the following before merging.
If applicable:

• All comments are resolved
• This is not your own PR
• All CIs are happy
• PR title starts with [MRG]
• whats_new.rst is updated
• New contributors have been added to CITATION.cff
• PR description includes phrase "closes <#issue-number>"

[hoechenberger]

New logic:

• If only events and event_id are passed, all remains unchanged
• If no events are passed, but the data has annotations, it's now possible (optional!) to specify event_id to allow for a custom mapping from annotation descriptions to event codes. If event_id is passed, though, it must contain all annoation descriptions. If it is not passed, we will assign arbitrary event codes to each annotation description. This is the old behavior, i.e., this bit remains unchanged; however, we now emit an info log message
• If events are passed and the data has annotations, we now require that event_id not only contain a mapping for all events, but also for all annotation descriptions, otherwise we raise an exception

[codecov]

Codecov Report

Merging #1086 (c4e17ab) into main (861ccd1) will decrease coverage by 0.33%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1086      +/-   ##
==========================================
- Coverage   95.20%   94.87%   -0.34%     
==========================================
  Files          24       24              
  Lines        3859     3860       +1     
==========================================
- Hits         3674     3662      -12     
- Misses        185      198      +13     

Impacted Files	Coverage Δ
mne_bids/write.py	96.69% <ø> (-0.51%)	⬇️
mne_bids/read.py	94.17% <100.00%> (-1.84%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[hoechenberger]

If we want to go a different route and allow for incomplete event_id mappings for annotations, we'll have to work upstream on MNE's events_from_annotations()

[hoechenberger]

MWE:

# %%
import pandas as pd
import mne
import mne_bids

sample_dir = mne.datasets.sample.data_path()
sample_fname = sample_dir / 'MEG' / 'sample' / 'sample_audvis_raw.fif'

raw = mne.io.read_raw_fif(sample_fname)
raw_concat = mne.concatenate_raws([raw.copy(), raw])
events = mne.find_events(raw_concat)
event_id = {
    'Auditory/Left': 1,
    'Auditory/Right': 2,
    'Visual/Left': 3,
    'Visual/Right': 4,
    'Smiley': 5,
    'Button': 32,
    'BAD boundary': 9999,
    'EDGE boundary': 10000,
}

# %%
bp = mne_bids.BIDSPath(
    subject='sample',
    task='sample',
    suffix='meg',
    extension='.fif',
    datatype='meg',
    root='/tmp/bids-root'
)
mne_bids.write_raw_bids(
    raw_concat,
    bids_path=bp,
    events=events,
    event_id=event_id,
    overwrite=True,
    allow_preload=True,
    format='FIF'
)

# %%
events_tsv_path = (
    bp
    .copy()
    .update(suffix='events', extension='.tsv')
)
events_tsv = pd.read_csv(events_tsv_path, sep='\t')
print(events_tsv['trial_type'].unique())

produces:

['Auditory/Right' 'Visual/Left' 'Auditory/Left' 'Visual/Right' 'Smiley'
 'Button' 'BAD boundary' 'EDGE boundary']

[hoechenberger]

I know this is complex stuff, but @sappelhoff if you have 20 min to review, it would be great :) because it's actually an annoying bug that ideally should get fixed soon

[sappelhoff]

I'll try to do it soon!

[sappelhoff]

LGTM, I like the new behavior! Thanks @hoechenberger!

[sappelhoff]

I added a versioning section and patch release section to the release wiki: https://github.com/mne-tools/mne-bids/wiki/How-to-make-a-release

and I'll look into filling up some more information there and making a patch release soon @hoechenberger