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

add ieeg visual_multimodal dataset #138

Merged
merged 20 commits into from
Mar 1, 2019

Conversation

irisgroen
Copy link
Contributor

@irisgroen irisgroen commented Dec 5, 2018

I've added a dataset of one patient with multimodal measurements (ECoG and fMRI) and multiple different visual tasks that were run in both modalities. The datafiles and stimulus files are all empty. I've tested the dataset on bids compatibility using the bids-validator with the -bep010 flag. The validator flags a couple of errors I am trying to get resolved by posting issues on the validator repo.
I hope this dataset will be useful as an example of how multi-modal data can be organized in a nice systematic way using BIDS. Please let me know if anything is missing or needs to be changed.

…s for the distortion scans (fmap folder of 3T fMRI session)
@irisgroen
Copy link
Contributor Author

irisgroen commented Dec 10, 2018

@choldgraf @dorahermes While writing down a description of these data for the BIDS spec document I realized a few things:

  • Wondering if it's really necessary to include all of our 5 different tasks, this makes the whole thing quite lengthy - perhaps a (somewhat) reduced version of the dataset would be more helpful (still multiple tasks and runs, but not all 5). Or do you prefer it being as 'close' to reality as possible?
  • I don't have a scans.tsv file, is that required when you have multiple runs or just recommended?
  • Is my anat folder with the pre-operative T1 actually in the right location? It was collected on a different day and at a separate location than the ECoG, which is why I put in a separate session (som3t01) but its purpose is for registering the electrode positions, and in some other examples this scan is within the same session as the 'ieeg' folder I think. Also not sure if there should be a .json with the pre-operative T1w.nii.gz (as we have for the T1 from the fMRI session)?

@dorahermes
Copy link
Member

Thank you!

  • Wondering if it's really necessary to include all of our 5 different tasks, this makes the whole thing quite lengthy - perhaps a (somewhat) reduced version of the dataset would be more helpful (still multiple tasks and runs, but not all 5). Or do you prefer it being as 'close' to reality as possible?
    -->multiple tasks and runs is really good, a reduced version is fine.

  • I don't have a scans.tsv file, is that required when you have multiple runs or just recommended?
    --> scans.tsv is optional

  • Is my anat folder with the pre-operative T1 actually in the right location? It was collected on a different day and at a separate location than the ECoG, which is why I put in a separate session (som3t01) but its purpose is for registering the electrode positions, and in some other examples this scan is within the same session as the 'ieeg' folder I think. Also not sure if there should be a .json with the pre-operative T1w.nii.gz (as we have for the T1 from the fMRI session)?
    --> I don't think a T1w needs a json to accompany it.
    --> I think that having the T1w in a different session if it was collected on a different day makes sense ("session = a logical grouping of neuroimaging and behavioural data consistent across subjects").

@irisgroen
Copy link
Contributor Author

irisgroen commented Dec 21, 2018

I've reduced the number of tasks and fixed the stimuli. I think the dataset is ready to be merged except that the bids-validator still shows one error about the columns of the channels.tsv files, relating to https://github.com/bids-standard/bids-validator/issues/666

@dorahermes
Copy link
Member

Thank you, lgtm!
@choldgraf @chrisfilo can we merge this?

@chrisgorgo
Copy link
Contributor

I don't think I have the expertise to review it but @choldgraf an @dorahermes should have merge rights to this repo.

@sappelhoff sappelhoff closed this Feb 25, 2019
@sappelhoff sappelhoff reopened this Feb 25, 2019
@sappelhoff
Copy link
Member

I closed and reopened to trigger a new Travis build ... just checking whether the updated config fixes the build. Fingers crossed :-)

@irisgroen
Copy link
Contributor Author

No luck with that build it seems, let me know if there's something I can do...

Also, I checked this dataset again against the newest version of bids-validator (1.1.5) and I get one new error, about the formatting of the _ieeg.json files:

bids-validator --bep010 ./ieeg_visual_multimodal/

1: [ERR] Invalid JSON file. The file is not formatted according the schema. (code: 55 - JSON_SCHEMA_VALIDATION_ERROR)
		./sub-som682/ses-somecog01/ieeg/sub-som682_ses-somecog01_task-spatialobject_run-01_ieeg.json
			Evidence:  should NOT have additional properties

It seems I have more fields in there than what is allowed, but now that the Google Doc with the BIDS-ieeg specifications is no longer actively maintained I'm not sure where to find the latest version of the BIDS-ieeg spec to double check the requirements, can someone direct me to that? @dorahermes?

@sappelhoff
Copy link
Member

No luck with that build it seems, let me know if there's something I can do...

This is a travis / installation issue of the validator, see: https://github.com/bids-standard/bids-validator/issues/723

I'm not sure where to find the latest version of the BIDS-ieeg spec to double check the requirements

The iEEG spec has been merged and is available in the "latest" version of the specification: https://bids-specification.readthedocs.io/en/latest/04-modality-specific-files/04-intracranial-electroencephalography.html

Note that this is not the "stable" version (i.e., it's not yet released). It will become stable in version 1.2.0, see: bids-standard/bids-specification#161

@sappelhoff sappelhoff closed this Feb 26, 2019
@sappelhoff sappelhoff reopened this Feb 26, 2019
@sappelhoff
Copy link
Member

okay, the build is running again, but visual_multimodal has some issues, as you already mentioned :-)

They should be straight forward to fix now!

Report: https://travis-ci.org/bids-standard/bids-examples/builds/498633328?utm_source=github_status&utm_medium=notification

@irisgroen
Copy link
Contributor Author

irisgroen commented Feb 26, 2019

OK, I fixed the last visual_multimodal issues and it now passes the validator in the Travis Build. So I think this dataset is ready to merge! The remaining failed checks are due to what seem to be fixable issues (compatibility with latest ieeg spec) in other example datasets (motorMiller and ieeg_visual).

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

Excellent!

Could you address the remaining two warnings?

Validating dataset ieeg_visual_multimodal/
1: [WARN] Tabular file contains custom columns not described in a data dictionary (code: 82 - CUSTOM_COLUMN_WITHOUT_DESCRIPTION)
./participants.tsv
Evidence: Columns: age, sex not defined, please define in: /participants.json

and also:

2: [WARN] The recommended file /README is missing. See Section 03 (Modality agnostic files) of the BIDS specification. (code: 101 - README_FILE_MISSING)

@sappelhoff
Copy link
Member

great! ... now you just need to add *_events.json files, in which you descibe the keys that you use in your *_events.tsv files.

onset duration ISI trial_type trial_name stim_file

This *_events.json file could in principle be written next to each of your events.tsv files ... however, if the keys are always the same for the participants, you can make use of the "Inheritance principle".

With the inheritance principle, you could just write a single events.json file defined at the root of your dataset. For an example where this is done, see: https://github.com/bids-standard/bids-examples/tree/bep006_eeg/eeg_matchingpennies

there, only a single events.json is defined which is then valid for all subject and session etc. wise events.tsv files.

@irisgroen
Copy link
Contributor Author

Thanks for pointing me to the inheritance principle and the matching pennies example dataset, that was very helpful! I actually would like to update the README according to the example there, so please let me do that first before merging.

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

the two comments I made (see below) refer to all events.json files. After making the changes there, you might have to change the keys in events.tsv as well.

@sappelhoff
Copy link
Member

Thanks for pointing me to the inheritance principle and the matching pennies example dataset, that was very helpful!

sure, I am happy to help :-)

and I also found some more things to be changed (see above). I'll not merge this without you being ready, no worries.

@irisgroen
Copy link
Contributor Author

I hope I now addressed your requests - but I think I actually introduced a new issue with the stimfile pointers in the events.tsv files, will try to solve that now and upload them again!

@sappelhoff
Copy link
Member

yes something went wrong with your events.json formatting. But that seems to be the only issue left!

@irisgroen
Copy link
Contributor Author

OK, I fixed that last error, and I actually changed the stimulus reference because I realized that what I had done before wasn't quite accurate - now it should be correct! The Travis Build says this:
Validating dataset ieeg_visual_multimodal/ This dataset appears to be BIDS compatible.
Yay! 💃

@dorahermes
Copy link
Member

Awesome! :-)
(tagging @choldgraf so that he can also share in the excitement)

@choldgraf
Copy link
Collaborator

wahooo!!! that is in fact a very satisfying success message to see after a lot of debugging time :-) way to go @irisgroen !

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

Wonderful, thank you @irisgroen

if you want to indicate that this can be merged, please edit the title of your PR to contain [MRG], or leave me a note.

@sappelhoff
Copy link
Member

I'll just merge for now, we can always address changes later.

@sappelhoff sappelhoff merged commit 21b8a39 into bids-standard:bep010_ieeg Mar 1, 2019
@irisgroen
Copy link
Contributor Author

Yes sorry for the delay, I was just going to correct some minor typos and add one more cite to the README but got side-tracked - it's not important so I can definitely do that at a later time.

@sappelhoff
Copy link
Member

A good time to do these final corrections would be when iEEG is officially part of the spec, which will most likely happen tomorrow or within the next few days!

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

Successfully merging this pull request may close these issues.

5 participants