[MRG+1] Add bep006 eeg #78

sappelhoff · 2018-09-07T12:34:31Z

This will introduce BEP_006 "EEG" to MNE-BIDS and close #34

Current state of the BEP_006: Google doc

BEP_006 Examples: https://github.com/incf/bids-examples/tree/bep006_eeg

The bids-validator can already deal with EEG data by including the bep006 flag such as: bids-validator --bep006 /path/to/dataset

To Do:

Make MNE-BIDS aware that there are more types of data next to MEG (contingent on [MRG] remove default value for kind #73)
- for now, keep using kind
Introduce all EEG supported file types and test that they are converted and written correctly
- BrainVision is a multi-file format: .vhdr, .eeg, .vmrk
- EEGLAB .set files might be accompanied by a .fdt file
Make an example for EEG to be posted in the gallery
skip coordsystem.json and electrodes.tsv for EEG until the handling of coordinates is resolved in MNE-Python (i.e., no rescaling of the coordinates read from a file: WIP: Fix topomap plotting for EEG mne-python#5472)
scans.tsv is equal across modalities, write it for all (so far it was only written for meg)
events.tsv has been adjusted already in [MRG] bug fixes, test additions, meg_bids -->mne_bids ... Paving the way for EEG inclusion #55, no changes
reformat sidecar.json to better cover all shared keys. Issue for EEG REQUIRED: EEGReference ... make it n/a until we find a good way to infer it from mne.raw
find solution for EDF data
- for mne.find_events, it wants a parameter initialEvent=True
- when being read, it wants preload=True

General list of things to implement

Must-haves in the future:

coordsystem.json for EEG
electrodes.tsv for EEG

Nice to haves ... but not too important and hard to implement

... for these, I currently do not have an idea how to extract the information automatically. Adding them as optional (None-defaulting) parameters would clutter the API - so I don't work on them for now.

Several sidecar.json fields in decreasing order of importance
- HardwareFilters
- ManufacturersModelName
- CapManufacturer
- CapManufacturersModelName

--> as a potential solution, see #47

jasmainak · 2018-09-07T14:55:45Z

@sappelhoff I didn't look closely but PR looks clean so far. Could you keep the diff small so it's easy to review? Thanks!

sappelhoff · 2018-09-08T10:57:10Z

So the skeleton to include EEG is ready. Please make a first round of reviews @jasmainak @teonbrooks @monkeyman192 so that I know what you consider a total no-go or for what you have better ideas.

What is still missing is

The example (ignore it for now - this is outdated)
Tests of
- the three new functions in utils.py I introduce
- actually converting one of each .vhdr, .edf, .set, .bdf, .cnt

PS:

Could you keep the diff small so it's easy to review?

This is really hard, because MNE-BIDS is quite MEG-centric and several changes have to be made to generalize to the other modalities

jasmainak · 2018-09-08T14:00:50Z

@sappelhoff what I'd really like to see even before examples is tests ...

jasmainak · 2018-09-08T14:03:03Z

examples/make_eeg_bids.py

+#        ---------- modality
+#            ---------- sub-xx_task-yy_modality.ext
+#            ---------- sub-xx_task-yy_modality.json
+#            ---------- sub-xx_task-yy_events.tsv


I would suggest removing L139--150. L153 will basically take care of it

jasmainak · 2018-09-08T14:07:09Z

examples/make_eeg_bids.py

+from mne.io import read_raw_brainvision
+
+from mne_bids import raw_to_bids
+from mne_bids.datasets import download_matchingpennies_subj


the function is missing right now

Also, if I remember correctly, this dataset was already in BIDS format. How do you propose to use it in the example in that case?

jasmainak · 2018-09-08T14:10:31Z

Diff looks clean so far :) Tests next.

jasmainak · 2018-09-08T14:31:05Z

examples/make_eeg_bids.py

+#
+# We are reading the data using MNE-Python's io module and the
+# `read_raw_brainvision` function, which takes a `.vhdr` file as an input.
+vhdr_path = os.path.join(data_dir, 'sub-05_task-matchingpennies_eeg.vhdr')


actually thinking a bit more, why not add an example for the physionet data: https://martinos.org/mne/dev/generated/mne.datasets.eegbci.load_data.html ? The fetcher already exists and there are many subjects (we could just fetch 2 or 3) -- so we can demonstrate how to do the conversion for more than n=1 subjects.

This is an excellent idea, I'm on it!

monkeyman192 · 2018-09-09T09:02:02Z

wrt the extra stuff in the sidecar.json, see #47
I think this is still something that could be good to do and would allow pretty much any extra data we want to add.

sappelhoff · 2018-09-10T14:52:29Z

EEGLAB bug needs to be solved to continue: mne-tools/mne-python#5503

jasmainak · 2018-09-10T18:27:22Z

okay I see! Feel free to ping me if you need a review on the PR to fix the bug

sappelhoff · 2018-09-10T20:00:49Z

mne_bids/mne_bids.py

@@ -253,7 +271,7 @@ def _scans_tsv(raw, raw_fname, fname, verbose):
    """
    # get MEASurement date from the data info
    meas_date = raw.info['meas_date']
-    if isinstance(meas_date, (np.ndarray, list)):
+    if isinstance(meas_date, (np.ndarray, list, tuple)):
        meas_date = meas_date[0]


In master of MNE-Python, meas_date has just been formatted to consistently be a tuple. So soon, we will be able to get rid of np.ndarray and list over there. Should I make a comment so we don't forget? mne-tools/mne-python#5500

I would make a comment and add an issue for the mne version that we will freeze for that particular release

mne_bids/tests/test_mne_bids.py

teonbrooks · 2018-09-11T22:16:07Z

examples/make_eeg_bids.py

@@ -0,0 +1,199 @@
+"""
+=========================
+Use MNE-Bids for EEG Data


nit: Bids -> BIDS

teonbrooks · 2018-09-11T22:16:14Z

examples/make_eeg_bids.py

+Use MNE-Bids for EEG Data
+=========================
+
+In this example, we use MNE-Bids to create a BIDS-compatible directory of EEG


teonbrooks · 2018-09-11T22:18:19Z

examples/make_eeg_bids.py

+# With this simple step we have everything to start a new BIDS directory using
+# our data. To do that, we can use the high level function `raw_to_bids`, which
+# is the core of MNE-BIDS. Generally, `raw_to_bids` tries to extract as much
+# meta data as possible from the raw data and then format it in a BIDS


format -> formats

mne_bids/mne_bids.py

teonbrooks · 2018-09-11T22:26:31Z

mne_bids/mne_bids.py

@@ -253,7 +271,7 @@ def _scans_tsv(raw, raw_fname, fname, verbose):
    """
    # get MEASurement date from the data info
    meas_date = raw.info['meas_date']
-    if isinstance(meas_date, (np.ndarray, list)):
+    if isinstance(meas_date, (np.ndarray, list, tuple)):
        meas_date = meas_date[0]


I would make a comment and add an issue for the mne version that we will freeze for that particular release

teonbrooks · 2018-09-11T22:33:55Z

mne_bids/mne_bids.py

-    # for FIF, we need to re-save the file to fix the file pointer
-    # for files with multiple parts
+    # Copy the imaging data files
+    # Re-save FIF files to fix the file pointer for files with multiple parts


we should a comment that this is not currently implemented. I am working on it with this pr: mne-tools/mne-python#5470

@teonbrooks I guess we would need a copyfile_fif function eventually? Or is your MNE PR going to take care of it?

teonbrooks · 2018-09-11T22:36:10Z

mne_bids/tests/test_mne_bids.py

+
+def test_set():
+    """Test raw_to_bids conversion for EEGLAB data."""
+    output_path = '/home/stefanappelhoff/Desktop/bogus'  # _TempDir()


guessing this path is temp for now?

yes, some internal testing - thanks for noting it

mne_bids/tests/test_utils.py

teonbrooks · 2018-09-11T22:47:54Z

mne_bids/tests/test_utils.py

+
+    # IO error testing
+    with pytest.raises(ValueError):
+        copyfile_brainvision(raw_fname, new_name+'.eeg')


whitespace around operator? not sure if this is a pep8 violation or just style

mmmh I'd say it's a violation. Curiously, my linter did not catch it (linter-flake8 in atom)

teonbrooks · 2018-09-11T22:48:44Z

mne_bids/tests/test_utils.py

+
+    # IO error testing
+    with pytest.raises(ValueError):
+        copyfile_eeglab(raw_fname, new_name+'.wrong')


same as above

teonbrooks · 2018-09-11T22:57:03Z

mne_bids/utils.py

+    # Make sure we are dealing with file names as is customary, not paths
+    # Paths are problematic when copying the files to another system. Instead,
+    # always use the file name and keep the file triplet in the same directory
+    assert os.sep not in eeg_file


can these paths be in different folder hierarchies at recording? if so, maybe we should have a better error handling here

For both BrainVision and EEGLAB we can have arbitrary pointers to files. When these files are written by e.g., BrainVision Recorder of saved with EEGLAB the file pointers will always be simply the file name and we assume the file that is being pointed to to be in the same directory.

However, a user might manually change the pointers and include a path instead of only the file name. That is something I have not yet encountered ... but it is technically possible.

teonbrooks · 2018-09-11T22:59:36Z

mne_bids/utils.py

+    fname_dest, ext_dest = _parse_ext(vhdr_dest)
+    if ext_src != ext_dest:
+        raise ValueError('Need to move data with same extension'
+                         ' but got {}, {}'.format(ext_src, ext_dest))


should include double quotes are {} to highlight str extensions

teonbrooks · 2018-09-11T23:07:46Z

mne_bids/utils.py

+
+    Notes
+    -----
+    NOT IMPLEMENTED YET. This function will abort upon the encounter of a .fdt


teonbrooks

looks good so far. are you going to wait til the eeglab files are supported first?

sappelhoff · 2018-09-12T06:57:46Z

thanks for the review @teonbrooks !

looks good so far. are you going to wait til the eeglab files are supported first?

No, I wouldn't: If users run into a problem with that, they can reconvert their .set+.fdt combination to be only a standalone .set. I would make this a separate issue once this PR is merged because solving this will involve vigorous MAT file handling via python.

sappelhoff · 2018-09-12T11:20:11Z

Note: This PR will need #84 to run successfully.

Also, the test_bti test will still fail - due to the Bids-validator issue AND our poor handling of BTi data. As I already suggested in #84, I suggest to solve that in a separate PR.

So now this PR is [MRG]

jasmainak · 2018-09-15T15:54:40Z

examples/make_eeg_bids.py

+for subj_idx in [1, 2]:
+    eegbci.load_data(subject=subj_idx,
+                     runs=tasks,
+                     path=mne_dir,


ah you did this. Okay, I can live with it. It's fine ...

jasmainak · 2018-09-15T15:56:44Z

Good to go from my end. @teonbrooks @choldgraf or @agramfort do you want to take a look before I merge?

agramfort · 2018-09-16T12:15:27Z

is it possible to see the rendered doc?

sappelhoff · 2018-09-16T13:58:14Z

yes - It's in the artifacts tab of the circle ci service:

https://119-89170358-gh.circle-artifacts.com/0/html/auto_examples/make_eeg_bids.html

agramfort · 2018-09-16T15:42:44Z

examples/make_eeg_bids.py

+# initial `dataset_description` on top!
+#
+# 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. these places are marked


these -> These

agramfort · 2018-09-16T15:43:04Z

mne_bids/io.py

+
+    # EDF (european data format) or BDF (biosemi) format
+    elif ext == '.edf' or ext == '.bdf':
+        raw = io.read_raw_edf(raw_fname, preload=True)


why is using preload=True necessary?

Because MNE-Python wants it so for the EDF reader, else you get:

RuntimeError: EDF+ Annotations (TAL) channel needs to be parsed completely on loading. You must set preload parameter to True.

ok fair enough.

I get we need to fix this in mne-python. We should not need to preload to get the raw.annotations available.

cc @massich

agramfort · 2018-09-16T15:43:16Z

examples/make_eeg_bids.py

+# MNE-Python's io module and the `read_raw_edf` function.
+data_dir = os.path.join(data_dir, 'physiobank', 'database', 'eegmmidb')
+edf_path = os.path.join(data_dir, 'S001', 'S001R02.edf')
+raw = read_raw_edf(edf_path, preload=True)


same here. Why preload=True?

see above, this is for all EDF files

jasmainak · 2018-09-16T21:32:27Z

alright, merged @sappelhoff ! Good way to start your week ;)

jasmainak · 2018-09-16T21:33:59Z

doc/whats_new.rst

@@ -15,6 +15,7 @@ Current
 Changelog
 ~~~~~~~~~

+- Add support for EEG and a corresponding example: `make_eeg_bids.py` by `Stefan Appelhoff`_ (`#78 <https://github.com/mne-tools/mne-bids/pull/78>`_)


oops, this should have been added at the bottom. Okay we can fix it later

teonbrooks · 2018-09-17T19:35:52Z

we have EEG support! 🎉 thanks @sappelhoff!

sappelhoff self-assigned this Sep 7, 2018

sappelhoff force-pushed the add_bep006_eeg branch from 0cf73db to 523a631 Compare September 7, 2018 12:36

jasmainak reviewed Sep 8, 2018

View reviewed changes

mne-tools deleted a comment from jasmainak Sep 8, 2018

sappelhoff mentioned this pull request Sep 9, 2018

[BUG] meas_date: list, int, tuple? mne-tools/mne-python#5499

Closed

sappelhoff commented Sep 10, 2018

View reviewed changes

mne_bids/tests/test_mne_bids.py Show resolved Hide resolved

monkeyman192 mentioned this pull request Sep 11, 2018

measurement date always tuple now #80

Closed

sappelhoff mentioned this pull request Sep 11, 2018

[BUG] _channels_tsv writes default unit for channel type #81

Closed