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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions examples/convert_eeg_to_bids.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,8 +228,8 @@
###############################################################################
# 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.
# describe EEGGround yourself. It's easy to find these by searching for "n/a"
# in the sidecar files.
#
# Remember that there is a convenient javascript tool to validate all your BIDS
# directories called the "BIDS-validator", available as a web version and a
Expand Down
4 changes: 2 additions & 2 deletions examples/convert_ieeg_to_bids.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,8 @@
#
# 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 iEEGReference and iEEGGround yourself. It's easy to find these by
# searching for "n/a" in the sidecar files.
# describe iEEGGround yourself. It's easy to find these by searching for "n/a"
# in the sidecar files.
#
# `$ grep -i 'n/a' <bids_root>`
#
Expand Down
47 changes: 47 additions & 0 deletions mne_bids/tests/test_write.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1245,6 +1245,53 @@ def test_eegieeg(dir_name, fname, reader, _bids_validate):
_bids_validate(output_path)


@pytest.mark.parametrize(
('eeg_reference', 'test_ieeg'),
[('FCz', True),
('Linked Ears', False),
(None, False),
(10, False)])
def test_eeg_reference(eeg_reference, test_ieeg, _bids_validate):
"""Ensure (i)EEGReference is written properly."""
bids_root = _TempDir()
bids_path = _bids_path.copy().update(root=bids_root, datatype='eeg')

dir_name, fname, reader = test_eegieeg_data[0] # EDF data
raw_fname = op.join(testing.data_path(), dir_name, fname)
raw = reader(raw_fname)

kwargs = dict(raw=raw, bids_path=bids_path, eeg_reference=eeg_reference,
overwrite=True)
if eeg_reference is None or eeg_reference == 10:
with pytest.raises(TypeError, match='must be an instance of string'):
write_raw_bids(**kwargs)
return

write_raw_bids(**kwargs)
_bids_validate(bids_root)

json_path = bids_path.copy().update(extension='.json', suffix='eeg')
with open(json_path, 'r', encoding='utf-8') as f:
json_dict = json.load(f)

assert 'EEGReference' in json_dict
assert json_dict['EEGReference'] == eeg_reference

if test_ieeg:
bids_path.update(datatype='ieeg')
raw.pick_types(meg=False, eeg=True)
raw.set_channel_types({name: 'ecog' for name in raw.ch_names})
write_raw_bids(**kwargs)
_bids_validate(bids_root)

json_path = bids_path.copy().update(extension='.json', suffix='ieeg')
with open(json_path, 'r', encoding='utf-8') as f:
json_dict = json.load(f)

assert 'iEEGReference' in json_dict
assert json_dict['iEEGReference'] == eeg_reference


def test_bdf(_bids_validate):
"""Test write_raw_bids conversion for Biosemi data."""
bids_root = _TempDir()
Expand Down
23 changes: 16 additions & 7 deletions mne_bids/write.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,8 +541,8 @@ def _mri_scanner_ras_to_mri_voxels(ras_landmarks, img_mgh):
return vox_landmarks


def _sidecar_json(raw, task, manufacturer, fname, datatype, overwrite=False,
verbose=True):
def _sidecar_json(raw, task, manufacturer, fname, datatype, eeg_reference,
overwrite=False, verbose=True):
"""Create a sidecar json file depending on the suffix and save it.

The sidecar json file provides meta data about the data
Expand All @@ -561,6 +561,8 @@ def _sidecar_json(raw, task, manufacturer, fname, datatype, overwrite=False,
Filename to save the sidecar json to.
datatype : str
Type of the data as in ALLOWED_ELECTROPHYSIO_DATATYPE.
eeg_reference : str
The (i)EEG referencing scheme.
overwrite : bool
Whether to overwrite the existing file.
Defaults to False.
Expand Down Expand Up @@ -627,12 +629,12 @@ def _sidecar_json(raw, task, manufacturer, fname, datatype, overwrite=False,
('MEGChannelCount', n_megchan),
('MEGREFChannelCount', n_megrefchan)]
ch_info_json_eeg = [
('EEGReference', 'n/a'),
('EEGReference', eeg_reference),
('EEGGround', 'n/a'),
('EEGPlacementScheme', _infer_eeg_placement_scheme(raw)),
('Manufacturer', manufacturer)]
ch_info_json_ieeg = [
('iEEGReference', 'n/a'),
('iEEGReference', eeg_reference),
('ECOGChannelCount', n_ecogchan),
('SEEGChannelCount', n_seegchan)]
ch_info_ch_counts = [
Expand Down Expand Up @@ -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',
Copy link
Member

@jasmainak jasmainak Feb 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use this function:

mne-bids/mne_bids/sidecar_updates.py

Line 16 in 35f3c3d

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice @adam2392 you are the best!

Copy link
Member Author

@hoechenberger hoechenberger Feb 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Remove writing 'n/a' to the fields to hack the passing of validator?
  2. 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

Copy link
Member Author

@hoechenberger hoechenberger Feb 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

overwrite=False, verbose=True):
"""Save raw data to a BIDS-compliant folder structure.

Expand Down Expand Up @@ -1003,7 +1005,6 @@ def write_raw_bids(raw, bids_path, events_data=None,
If ``False`` (default), all subject information next to the
recording date will be overwritten as well. If True, keep subject
information apart from the recording date.

format : 'auto' | 'BrainVision' | 'FIF'
Controls the file format of the data after BIDS conversion. If
``'auto'``, MNE-BIDS will attempt to convert the input data to BIDS
Expand All @@ -1012,6 +1013,11 @@ def write_raw_bids(raw, bids_path, events_data=None,
the original file format lacks some necessary features. When a str is
passed, a conversion can be forced to the BrainVision format for EEG,
or the FIF format for MEG data.
eeg_reference : str
The electrode(s) or referencing scheme used for (i)EEG recordings, for
example: ``'Cz'``, ``'single electrode placed on FCz'``, or
``'left mastoid'``. Will only be considered if the data actually
contains (i)EEG recordings.
overwrite : bool
Whether to overwrite existing files or data in files.
Defaults to ``False``.
Expand Down Expand Up @@ -1090,6 +1096,9 @@ def write_raw_bids(raw, bids_path, events_data=None,
item_name='events_data',
type_name='path-like, NumPy array, or None')

_validate_type(eeg_reference, types='str', item_name='eeg_reference',
type_name='string')

# Check if the root is available
if bids_path.root is None:
raise ValueError('The root of the "bids_path" must be set. '
Expand Down Expand Up @@ -1246,7 +1255,7 @@ def write_raw_bids(raw, bids_path, events_data=None,
verbose=verbose)

_sidecar_json(raw, bids_path.task, manufacturer, sidecar_path.fpath,
bids_path.datatype, overwrite, verbose)
bids_path.datatype, eeg_reference, overwrite, verbose)
_channels_tsv(raw, channels_path.fpath, overwrite, verbose)

# create parent directories if needed
Expand Down