RFC, WIP: Add eeg_reference kwarg to write_raw_bids() #695

hoechenberger · 2021-02-07T11:10:48Z

PR Description

This PR adds an eeg_reference kwarg to write_raw_bids(), allowing users to set EEGReference (EEG) and iEEGReference (iEEG).

Con:

growing API
where do we stop? there's like a million optional params for different recording types; we cannot add all of them as kwargs

Pro:

an EEG reference is such a fundamental property of a recording, it must be extremely convenient to add it to the data in a single go without having to manually fiddle with the JSON sidecars, as is currently suggested in the docs:

# Now it's time to manually check the BIDS directory and the meta files to add
# all the information that MNE-BIDS could not infer. For instance, you must
# describe EEGReference and EEGGround yourself. It's easy to find these by
# searching for "n/a" in the sidecar files.

We need a general discussion about this at one point; however I am deeply convinced that for now, having an eeg_reference param in write_raw_bids() is the best option. But you're free to try and change my mind ;)

😅

Merge checklist

Maintainer, please confirm the following before merging:

sappelhoff

agreed with your pro and con, given that EEGReference is required

hoechenberger · 2021-02-07T11:27:40Z

given that EEGReference is required

I should point out that we always wrote it in the past, but set it to 'n/a' – kind of a dirty workaround to produce BIDS-compliant datasets

agramfort · 2021-02-07T17:15:02Z

what bothers me is that the raw.info should contain this information and in an ideal world we should not need to add any parameter to write_raw_bids...

can we start by seeing how to write properly mne data on which set_eeg_reference has been applied?

hoechenberger · 2021-02-07T17:26:01Z

can we start by seeing how to write properly mne data on which set_eeg_reference has been applied?

Yes we could do that. This was also my very first idea. I ditched / postponed it because that's typically not how things work in EEG:

At least with the systems I've worked with in the past, the recorded data would typically not contain the reference electrode by default – it would simply be missing. The signals of other electrodes, however, would readily be referenced to the (missing) reference.

Now if you were to write these data to BIDS immediately, you'd need to have a way to specify what the reference was even though it's now missing.

In MNE, I'd typically add this channel back with all zeros via add_reference_channels(). Question is whether this would already be a derivative.

jasmainak · 2021-02-07T19:13:52Z

mne_bids/write.py

@@ -903,7 +905,7 @@ def make_dataset_description(path, name, data_license=None,

 def write_raw_bids(raw, bids_path, events_data=None,
                   event_id=None, anonymize=None,
-                   format='auto',
+                   format='auto', eeg_reference='n/a',


I can live with it but not super enthusiastic

Instead I'd suggest having an API to find the 'n/a' entries in a dataset, return them as a dict and update them across the dataset. Didn't we have an issue somewhere? We can't have a param for every entry in the json files ...

na_entries = find_entities(bids_root, cond='n/a') na_entries.update(eeg_reference=eeg_reference) save_entities(bids_root, na_entries)

Could use this function:

mne-bids/mne_bids/sidecar_updates.py

Line 16 in 35f3c3d

def update_sidecar_json(bids_path, entries, verbose=True):

nice @adam2392 you are the best!

Yes, I'm aware of this function and it's already mentioned in the documentation.

My opinion is that for this specific use case – adding an EEG reference – users shouldn't have to do additional work after calling write_raw_bids() – partly because it's inconvenient, partly because some will simply forget about it (?)

Given there's such a plethora of potential parameters – and we cannot expect users to read the BIDS specification! – I'm wondering whether it could also make sense to have some kind of functionality that specifically helps with adding the REQUIRED and RECOMMENDED fields.

Look at the specs for *_eeg.json, for example:
https://bids-specification.readthedocs.io/en/stable/04-modality-specific-files/03-electroencephalography.html#sidecar-json-_eegjson

This list is sheer endless, and currently MNE-BIDS only fills out a few RECOMMENDED fields – and some of the REQUIRED fields are simply set to 'n/a' to make the validator pass. That's not how things should be. And like I said, we shouldn't expect users to read the specs, esp. since they can change from revision to revision. We need to give users a hand.

I think this EEGLAB tool does a way better job at guiding users than we do:
https://github.com/sccn/bids-matlab-tools/wiki#export-datasets-to-bids-from-the-graphic-interface

I wonder if we need something similar to make all those BIDS options really accessible to our users.

okay fair enough. But is EEG reference special in any way from the other parameters? I totally agree that we need to hold the user's hand as much as we can. Would it make sense to:

Remove writing 'n/a' to the fields to hack the passing of validator?

Then add a function ... or maybe a new BIDS object (like the Info object in MNE) that prepopulates the fields? E.g., if you do:

BIDSTemplate(data_type='eeg')

you will get a dict with empty fields. But if you do:

BIDSTemplate(bids_root)

you will get a dict with fields populated with what is in the dataset. And then you can fix the examples so they explicitly update the required fields

you will get a dict with fields populated with what is in the dataset. And then you can fix the examples so they explicitly update the required fields

This is getting a little more complex if we consider that each participant and session can have different settings… Also it would require you to already have a BIDS dataset.

Since (i)EEGReference is a REQUIRED field, then it makes sense to allow that to be passed to write_raw_bids, so I am more agreeing with @hoechenberger now... Are there many more REQUIRED fields that we aren't explicitly writing?

Other ideas

For more advanced usage, would we one day want to allow just **kwargs arguments to write_raw_bids, where a user can pass ANY REQUIRED fields to overwrite?

Another idea that is probably unwieldy but will throw out there :p, could we create a dictionary template with the default values for all the REQUIRED and RECOMMENDED fields for MEG/EEG/iEEG and have the user import them, set certain fields themselves and then pass to write_raw_bids?

jasmainak · 2021-02-07T19:19:01Z

Question is whether this would already be a derivative.

The definition of derivatives is somewhat based on use-case. Is there a case where the user may need to go to the previous data and change the processing for some reason? If the answer is no, you can distribute it as source data IMO :)

hoechenberger · 2021-02-25T20:04:36Z

I have decided that the best solution would be to close this PR and instead amend our documentation, suggesting users do the following before calling write_raw_bids:

if the reference channels are present in the data, call raw.set_eeg_reference() to make the reference explicitly known
if the reference channels are not present in the data:
- add them via raw.add_reference_channels()
- call raw.set_eeg_reference() to make the reference explicit

The cool thing would be that if the reference electrode is part of the montage, we'd get the reference electrode location for free :)

Then call write_raw_bids(), which should extract the reference channel info from raw.info['chs'] -> this will need a PR
Easiest is to start with the assertion that all channels have the same reference, so we can write it to [i]EEGReference in the sidecar JSON.

hoechenberger · 2023-09-14T16:47:24Z

Since we're still blocked by upstream limitations (MNE doesn't keep track of which custom reference was applied), I'm inclined to revive this PR...

larsoner · 2023-09-14T16:52:38Z

MNE doesn't keep track of which custom reference was applied

In principle this should be fixable. info['chs'][ii]['loc'][3:6] is supposed to store the position of the reference electrode. So that could be checed against the info['chs'][jj]['loc'][0:3] (or info['dig']) to figure out which electrode in the dataset is the reference. This will be a decent amount of effort to get right but I think it would work.

hoechenberger · 2023-09-14T16:56:23Z

Yes, I believe this was the approach you proposed a couple of years back, but nobody has gotten around implementing it yet. So I was wondering if it could make sense to move forward here (MNE-BIDS) to provide a solution, even if it's not the "best"

hoechenberger requested review from agramfort and sappelhoff February 7, 2021 11:10

WIP: Add eeg_reference kwarg to write_raw_bids()

6a942c8

hoechenberger force-pushed the eegreference branch from dbe6fd7 to 6a942c8 Compare February 7, 2021 11:22

sappelhoff reviewed Feb 7, 2021

View reviewed changes

jasmainak reviewed Feb 7, 2021

View reviewed changes

hoechenberger closed this Feb 25, 2021

hoechenberger mentioned this pull request Feb 25, 2021

Write EEG/iEEG reference to JSON sidecar #713

Open

hoechenberger deleted the eegreference branch June 22, 2021 08:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC, WIP: Add eeg_reference kwarg to write_raw_bids() #695

RFC, WIP: Add eeg_reference kwarg to write_raw_bids() #695

hoechenberger commented Feb 7, 2021

sappelhoff left a comment

hoechenberger commented Feb 7, 2021

agramfort commented Feb 7, 2021

hoechenberger commented Feb 7, 2021 •

edited

Loading

jasmainak Feb 7, 2021 •

edited

Loading

jasmainak Feb 7, 2021

adam2392 Feb 7, 2021

jasmainak Feb 7, 2021

hoechenberger Feb 8, 2021 •

edited

Loading

jasmainak Feb 8, 2021

hoechenberger Feb 8, 2021 •

edited

Loading

adam2392 Feb 8, 2021

jasmainak commented Feb 7, 2021

hoechenberger commented Feb 25, 2021 •

edited

Loading

hoechenberger commented Sep 14, 2023 •

edited

Loading

larsoner commented Sep 14, 2023

hoechenberger commented Sep 14, 2023

RFC, WIP: Add eeg_reference kwarg to write_raw_bids() #695

RFC, WIP: Add eeg_reference kwarg to write_raw_bids() #695

Conversation

hoechenberger commented Feb 7, 2021

PR Description

Merge checklist

sappelhoff left a comment

Choose a reason for hiding this comment

hoechenberger commented Feb 7, 2021

agramfort commented Feb 7, 2021

hoechenberger commented Feb 7, 2021 • edited Loading

jasmainak Feb 7, 2021 • edited Loading

Choose a reason for hiding this comment

jasmainak Feb 7, 2021

Choose a reason for hiding this comment

adam2392 Feb 7, 2021

Choose a reason for hiding this comment

jasmainak Feb 7, 2021

Choose a reason for hiding this comment

hoechenberger Feb 8, 2021 • edited Loading

Choose a reason for hiding this comment

jasmainak Feb 8, 2021

Choose a reason for hiding this comment

hoechenberger Feb 8, 2021 • edited Loading

Choose a reason for hiding this comment

adam2392 Feb 8, 2021

Choose a reason for hiding this comment

Other ideas

jasmainak commented Feb 7, 2021

hoechenberger commented Feb 25, 2021 • edited Loading

hoechenberger commented Sep 14, 2023 • edited Loading

larsoner commented Sep 14, 2023

hoechenberger commented Sep 14, 2023

hoechenberger commented Feb 7, 2021 •

edited

Loading

jasmainak Feb 7, 2021 •

edited

Loading

hoechenberger Feb 8, 2021 •

edited

Loading

hoechenberger Feb 8, 2021 •

edited

Loading

hoechenberger commented Feb 25, 2021 •

edited

Loading

hoechenberger commented Sep 14, 2023 •

edited

Loading