corrected meg events onsets#27
Conversation
| @@ -1,147 +1,147 @@ | |||
| onset duration onset_sample stim_type trigger stim_file | |||
| 26.078 0 26628 Unfamiliar 13 meg/u032.bmp | |||
There was a problem hiding this comment.
why is duration = 0? In the publication, it says: "the critical stimulus (face or scrambled face) was superimposed for a random duration between 800 and 1,000 ms."
And in the BIDS specification, it says: "A 'duration' value of zero implies that the delta function or event is so short as to be effectively modeled as an impulse."
There was a problem hiding this comment.
@jbwexler, would you care considering the question about the duration?
There was a problem hiding this comment.
@robertoostenveld Sorry, we had been discussing this over email since not everyone is on github. I'll foward it to you.
|
I take it that 1100 is the sampling frequency. Should the first sample in the file not be at offset 0.000 (time in seconds)? If you count samples starting at 1 (rather than 0) as is common in MATLAB, that would imply that onset_time = (onset_sample-1)/1100 |
|
Is this PR likely to merged soon? Has someone tried running the validator on it? I found the following problems:
Here is the full traceback if you're interested. |
| 33.3618 0 36698 Unfamiliar 13 meg/u084.bmp | ||
| 36.5536 0 40209 Famous 5 meg/f123.bmp | ||
| 39.5764 0 43534 Unfamiliar 13 meg/u022.bmp | ||
| 42.8 0 47080 Famous 5 meg/f094.bmp |
There was a problem hiding this comment.
are you sure these onsets are correct? Can you share the scripts you used to compute them? Thanks
There was a problem hiding this comment.
Hmm, I don't have the scripts, but basically each onset is equal to [(Rik's onset) + 550]/1100. For sub-01 run-01, here is Rik's file: ftp://ftp.mrc-cbu.cam.ac.uk/personal/rik.henson/wakemandg_hensonrn/Sub01/MEEG/Trials/run_01_trldef.txt
So for line 1, Rik's onset was 26078. Thus [(26078) + 550]/1100 = 24.2073, which is the onset on line 1 of my file. Does that sound right? How do yours differ?
There was a problem hiding this comment.
hmm ... I don't know how these were found. What is 550 here? Did you look at the trigger channels? Here it is for the first file:
Don't you think the onset is less than 24 s?
There was a problem hiding this comment.
Yes, it does look like its less than 24. I'm not actually sure what 550 means but this is how Vladimir and Rik instructed me to calculate them. They're not on github AFAIK but I'll forward them this discussion so we can discuss over email.
|
another issue that I discovered due to the validator is that some cells in the tsv file are empty. The right way to label missing values in BIDS is |
|
i'm trying to find tsv files with missing cells. could you share an example? |
|
See for example here, rows 385 to 405 in the column "description" |
|
i see, thanks. I'll add a commit that fixes these |
|
okay cool, let me know when you push :) |
|
k, see the latest commit |
|
great! what about the other two comments? The headers in 'sub-01/ses-meg/beh/sub-01_ses-meg_task-facerecognition_events.tsv' and the missing stimuli files? |
|
Regarding the headers, Rik doesn't have the onsets and durations for the behavioral files so not much can be done about that. As for the stimuli, thanks for pointing that out. I'll see if I can find those missing images. Let me know if you come across other bugs. |
|
Thanks @jbwexler I'll let you know. I saw that you raised an issue on the mailing list. |
|
@jbwexler, please let us know when this PR is ready to be merged. |
|
K, I'll let you know. It should be in the next few days |
@jbwexler I just started back on the validator and I hit this issue again. Could you perhaps put n/a in these files if the information is not available? Thanks |
|
Thanks, we were planning to change the bids spec to allow this but actually using n/a is pretty good solution. At least for now I'll do that. |
|
@jbwexler thanks a lot! Another fix request: 2: JSON file is not formatted according the the schema. (code: 55 - JSON_SCHEMA_VALIDATION_ERROR)
./sub-01/ses-meg/meg/sub-01_ses-meg_task-facerecognition_fid.json
Evidence: should have required property 'LandmarkCoordinateUnits'Same for |
|
@jbwexler here is the output of the validator: 2: JSON file is not formatted according the the schema. (code: 55 - JSON_SCHEMA_VALIDATION_ERROR)
./sub-01/ses-meg/meg/sub-01_ses-meg_task-facerecognition_fid.json
Evidence: should have required property 'LandmarkCoordinates'
./sub-01/ses-meg/meg/sub-01_ses-meg_task-facerecognition_fid.json
Evidence: should have required property 'LandmarkCoordinateSystem'
./sub-01/ses-meg/meg/sub-01_ses-meg_task-facerecognition_fid.json
Evidence: should have required property 'MEGCoordinateSystem'
./sub-01/ses-meg/meg/sub-01_ses-meg_task-facerecognition_fid.json
Evidence: should have required property 'MEGCoordinateUnits'
./sub-01/ses-meg/meg/sub-01_ses-meg_task-facerecognition_fid.json
Evidence: should have required property 'EEGCoordinateSystem'
./sub-02/ses-meg/meg/sub-02_ses-meg_task-facerecognition_fid.json
Evidence: should have required property 'LandmarkCoordinates'
./sub-02/ses-meg/meg/sub-02_ses-meg_task-facerecognition_fid.json
Evidence: should have required property 'LandmarkCoordinateSystem'
./sub-02/ses-meg/meg/sub-02_ses-meg_task-facerecognition_fid.json
Evidence: should have required property 'MEGCoordinateSystem'
./sub-02/ses-meg/meg/sub-02_ses-meg_task-facerecognition_fid.json
Evidence: should have required property 'MEGCoordinateUnits'
./sub-02/ses-meg/meg/sub-02_ses-meg_task-facerecognition_fid.json
Evidence: should have required property 'EEGCoordinateSystem'
... and 70 more files having this issue (Use --verbose to see them all). |
|
Are there any missing fields I need to add other than LandmarkCoordinates, LandmarkCoordinateSystem, MEGCoordinateSystem, MEGCoordinateUnits and EEGCoordinateSystem? |
|
Doesn't seem like it, but here is the list for all the files |
|
Is this ready to be merged? |
|
@jbwexler are you happy with the changes? Does it pass the validator? |
|
@jasmainak please rebase the MEG branch of the validator. There are some significant changes in how unknown files are handled which might expose previously looked over issues. |
|
@jbwexler here is what the validator gives me after rebase: 1: Files with such naming scheme are not part of BIDS specification. This error is most commonly caused by typos in file names that make them not BIDS compatible. Please consult the specification and make sure your files are named correctly. If this is not a file naming issue (for example when including files not yet covered by the BIDS specification) you should include a ".bidsignore" file in your dataset. Please note that derived (processed) data should be placed in /derivatives folder and source data (such as DICOMS or behavioural logs in proprietary formats) should be placed in the /sourcedata folder. (code: 1 - NOT_INCLUDED)
./run-1_echo-1_FLASH.json
Evidence: run-1_echo-1_FLASH.json
./run-1_echo-2_FLASH.json
Evidence: run-1_echo-2_FLASH.json
./run-1_echo-3_FLASH.json
Evidence: run-1_echo-3_FLASH.json
./run-1_echo-4_FLASH.json
Evidence: run-1_echo-4_FLASH.json
./run-1_echo-5_FLASH.json
Evidence: run-1_echo-5_FLASH.json
./run-1_echo-6_FLASH.json
Evidence: run-1_echo-6_FLASH.json
./run-1_echo-7_FLASH.json
Evidence: run-1_echo-7_FLASH.json
./run-2_echo-1_FLASH.json
Evidence: run-2_echo-1_FLASH.json
./run-2_echo-2_FLASH.json
Evidence: run-2_echo-2_FLASH.json
./run-2_echo-3_FLASH.json
Evidence: run-2_echo-3_FLASH.json
... and 246 more files having this issue (Use --verbose to see them all).
2: This file is too small to contain the minimal NIfTI header. (code: 36 - NIFTI_TOO_SMALL)
./sub-01/ses-mri/anat/sub-01_ses-mri_acq-epi_T1w.nii
./sub-01/ses-mri/anat/sub-01_ses-mri_acq-mprage_T1w.nii
./sub-01/ses-mri/dwi/sub-01_ses-mri_dwi.nii
./sub-01/ses-mri/fmap/sub-01_ses-mri_magnitude1.nii
./sub-01/ses-mri/fmap/sub-01_ses-mri_magnitude2.nii
./sub-01/ses-mri/fmap/sub-01_ses-mri_phasediff.nii
./sub-01/ses-mri/func/sub-01_ses-mri_task-facerecognition_run-01_bold.nii
./sub-01/ses-mri/func/sub-01_ses-mri_task-facerecognition_run-02_bold.nii
./sub-01/ses-mri/func/sub-01_ses-mri_task-facerecognition_run-03_bold.nii
./sub-01/ses-mri/func/sub-01_ses-mri_task-facerecognition_run-04_bold.nii
... and 225 more files having this issue (Use --verbose to see them all).
1: Not all subjects contain the same files. Each subject should contain the same number of files with the same naming unless some files are known to be missing. (code: 38 - INCONSISTENT_SUBJECTS)
./sub-07/ses-mri/dwi/sub-07_ses-mri_dwi.bval
./sub-07/ses-mri/dwi/sub-07_ses-mri_dwi.bvec
./sub-07/ses-mri/dwi/sub-07_ses-mri_dwi.json
./sub-07/ses-mri/dwi/sub-07_ses-mri_dwi.nii
./sub-08/ses-mri/dwi/sub-08_ses-mri_dwi.bval
./sub-08/ses-mri/dwi/sub-08_ses-mri_dwi.bvec
./sub-08/ses-mri/dwi/sub-08_ses-mri_dwi.json
./sub-08/ses-mri/dwi/sub-08_ses-mri_dwi.nii
./sub-10/ses-mri/dwi/sub-10_ses-mri_dwi.bval
./sub-10/ses-mri/dwi/sub-10_ses-mri_dwi.bvec
... and 10 more files having this issue (Use --verbose to see them all).
Summary: Available Tasks: Available Modalities:
1958 Files, 3.87MB face recognition T1w
16 - Subjects dwi
2 - Sessions bold
fieldmap |
|
Are all of the NOT_INCLUDED errors related to FLASH (could you run int with --verbose)? |
|
Nopes, all the NOT_INCLUDED errors are not related to flash. For example, I also see: task-facerecognition_fidinfo.pdf giving the same error. |
|
Ok - thanks for checking. If |
|
nopes, it's not part of the extension |
|
Since those are suppose to be examples I would opt for removing those pdfs then. |
|
Finally, fidinfo.pdf is not part of the specs. We have included it as a new field inf the old *_fid.json. Now *_fid.json has been renamed to *_coordsystem.json |
|
@chrisfilo I can also make a PR on |
|
Up to you - I thought it's worth while keeping this one up because of all the associated discussion. Let me know how would you like me to proceed. |
|
yep, let's merge this. One branch less to worry about :) |
|
Thanks @chrisfilo ! |
corrected meg events onsets Former-commit-id: 113704a
Rik and Vladimir pointed out that I had calculated the onsets incorrectly. Now onset = onset_sample/1100.