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.

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

read_raw_bids does not use value column from events file #223

Open
hstojic opened this issue Jul 12, 2019 · 13 comments
Open

read_raw_bids does not use value column from events file #223

hstojic opened this issue Jul 12, 2019 · 13 comments
Milestone

Comments

@hstojic
Copy link

hstojic commented Jul 12, 2019

read_raw_bids seems to define its own values for events.

For example, in the event file I have a value column with events specified as 1,2 and 4 with trial_type defining the names, but then when data is read in they are redefined as 1,2,3 in alphabetical order.

If the (optional) value column is present the reader should use those values instead of imposing generic ones.

@sappelhoff
Copy link
Member

Thanks for reporting! I assume you are using mne_bids with version 0.2 and NOT the development version? You can check your version by calling mne_bids --version from the command line.

First of all: I agree with you, in your case read_raw_bids SHOULD use the event values as defined in the value column of your events.tsv file.

That said, read_raw_bids will handle events slightly differently in the next release (0.3):

Instead of returning raw, events, and events_id - read_raw_bids will ONLY return raw in the future.

However, this raw object will have an attribute: raw.annotations, which will give you Annotations, which is a relatively new feature in MNE to handle events. You can check out a nice tutorial here.

If you want events and event_id in the future, you will have to call mne.events_from_annotations

It'll look a bit like this:

import mne
from mne_bids import read_raw_bids

raw = read_raw_bids('./my_bids_fname', './my_bids_root')
events, event_id = mne.events_from_annotations(raw)
# note, events_id is NOT as in events.tsv
# rather, event_id will be consecutive numbers, see the docstring of events_from_annotations

... no where does that leave us with MNE-BIDS being capable to transfer your values from the value column in your events.tsv to Python and MNE? ... I don't know.

Some options:

  • make a function event_id = mne_bids.get_event_id(bids_fname)
  • make read_raw_bids return not only raw, but raw AND event_id ... then one could do:
import mne
from mne_bids import read_raw_bids

raw, events_id = read_raw_bids('./my_bids_fname', './my_bids_root')
# events_id is according to events.tsv!
events, _ = mne.events_from_annotations(raw)
# use the _ ... because otherwise we would get an events_id that we already have

What are your opinions? Are there more options that I did not mention?

cc @jasmainak @agramfort

@jasmainak
Copy link
Member

I thought trial_type was meant to store the text description of events not an event code. So it's more like the key of an event_id dictionary rather than the value. Do I miss something?

@sappelhoff
Copy link
Member

Do I miss something?

no, you are right ... but there is a different column in events.tsv files called value (see the table) which refers to the actual value of the TTL trigger / event marker in the raw data.

MNE will give each description (key in event_it) a value ... but this value SHOULD correspond to the value column in events.tsv

@hstojic
Copy link
Author

hstojic commented Jul 13, 2019

Hmm, I'm not sure I follow the bit about annotations - does that mean when writing the BIDS data event files will be written directly in the raw data? If not, we would still need separate event files and load them when reading in the raw data, attaching them as annotations as new MNE would expect, no?

I used the development version actually, because 0.2 had an error with session names, so instead of just checking out that patch I went for the development version.

@jasmainak
Copy link
Member

does that mean when writing the BIDS data event files will be written directly in the raw data?

no, but I agree this is confusing. The confusion arises from the fact that events array is used to write events.tsv but when reading it in, the data goes into raw.annotations. This leads to an inconsistent API in my opinion. This can be fixed by either: a) writing also with raw.annotations b) reverting to returning the events array when reading the raw data. Pinging @agramfort for opinion

@agramfort
Copy link
Member

agramfort commented Jul 14, 2019 via email

@jasmainak
Copy link
Member

what's the consensus here? Do we still have an issue?

@agramfort
Copy link
Member

agramfort commented Jul 18, 2019 via email

@jasmainak
Copy link
Member

@hstojic do you agree? Closing issue, feel free to reopen if you disagree

@hstojic
Copy link
Author

hstojic commented Jul 19, 2019

Suppose I have my own coding scheme for triggers, put them in value column, and rely on them, not event names - in the scripts then if you use event values programmatically when loading various tasks that have some overlap in triggers, you would get errors or worse, accidentally it would find those values but it wouldn't be what you think it is.

Probably it's not going to be many such cases, but to be on the safe side ti does't hurt to use the value column numbers - there is no downside to it, right? Defensive programming

@sappelhoff
Copy link
Member

I agree @hstojic.

When reading a BIDS events.tsv we should use the values column as the "value" in a potential events_id dictionary ... and the trial_type column as the "key" in that dictionary.

@sappelhoff sappelhoff reopened this Jul 19, 2019
@hoechenberger hoechenberger added this to the 0.9 milestone Nov 8, 2021
@hoechenberger
Copy link
Member

hoechenberger commented Nov 9, 2021

I just ran into this issue while playing with the new anonymize_dataset() function: event trigger values changed in the anonymized data, which lead me to this issue. Here's a MWE that demonstrates the underlying problem:

# %%
from pathlib import Path
import numpy as np
import mne
import mne_bids

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

raw = mne.io.read_raw_fif(sample_fname)
events = mne.find_events(raw, stim_channel='STI 014')
event_id = {'visual/left': 3, 'visual/right': 4, 'face': 5, 'buttonpress': 32}
# Drop unused events
events = events[np.in1d(events[:, -1], list(event_id.values()))]

bp = mne_bids.BIDSPath(
    subject='foo', task='av', suffix='meg', extension='.fif', datatype='meg',
    root=Path('/tmp/bids-root')
)
mne_bids.write_raw_bids(
    raw=raw,
    bids_path=bp,
    events_data=events,
    event_id=event_id,
    overwrite=True
)
raw_read = mne_bids.read_raw_bids(bids_path=bp)
events_read, event_id_read = mne.events_from_annotations(raw=raw_read)

# %%
print('Original event_id:\n', event_id)
print('Round-tripped event_id:\n', event_id_read)

Produces:

Original event_id:
 {'visual/left': 3, 'visual/right': 4, 'face': 5, 'buttonpress': 32}
Round-tripped event_id:
 {'buttonpress': 1, 'face': 2, 'visual/left': 3, 'visual/right': 4}

I'm afraid the only way to fix this is to allow read_raw_bids to return an event_id dict which can then be passed to events_from_annotations() – or we add our own get_bids_events() function or something?

@hoechenberger
Copy link
Member

hoechenberger commented Nov 9, 2021

I'm afraid the only way to fix this is to allow read_raw_bids to return an event_id dict which can then be passed to events_from_annotations()

Or we add this to BIDSPath?

BIDSPath.events
BIDSPath.event_id

(both as @propertys)

And it does the right thing?
This would also free users from having to use events_from_annotations()?

@hoechenberger hoechenberger modified the milestones: 0.9, 0.10 Nov 14, 2021
hoechenberger added a commit to hoechenberger/mne-bids that referenced this issue Nov 17, 2021
Previously, anonymization would cause a new assignment of trigger
codes, which could lead to confusion and inconsistencies across
participants – or even within participants across runs.

We now simply overwrite the trigger codes in the anonymized
*_events.tsv files with the original codes.

This problem is directly related to mne-tools#223, and the solution implemented
here is merely a workaround (which is good enough for now)
agramfort pushed a commit that referenced this issue Nov 17, 2021
Previously, anonymization would cause a new assignment of trigger
codes, which could lead to confusion and inconsistencies across
participants – or even within participants across runs.

We now simply overwrite the trigger codes in the anonymized
*_events.tsv files with the original codes.

This problem is directly related to #223, and the solution implemented
here is merely a workaround (which is good enough for now)
@hoechenberger hoechenberger modified the milestones: 0.10, 0.11 Mar 2, 2022
@hoechenberger hoechenberger modified the milestones: 0.11, 0.12 Oct 6, 2022
@sappelhoff sappelhoff modified the milestones: 0.12, 0.13 Dec 18, 2022
@sappelhoff sappelhoff removed this from the 0.13 milestone Jul 27, 2023
@sappelhoff sappelhoff added this to the 0.14 milestone Jul 27, 2023
@sappelhoff sappelhoff modified the milestones: 0.14, 0.15 Dec 9, 2023
@sappelhoff sappelhoff modified the milestones: 0.15, 0.16 May 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants