Closed sjeung closed 1 year ago
Thank you
@dungscout96 the changes look ok to me, although you might want to look deeper at the changes related to channel location. @sjeung why did you modify the section importing channel location?
@dungscout96 the changes look ok to me, although you might want to look deeper at the changes related to channel location. @sjeung why did you modify the section importing channel location?
Awesome, thanks Arno, I suggest the change because electrodes.tsv is optional in BIDS and I noticed that the chanloc part would throw an error when the file is not found. I also have to polish the metadata storage because currently almost nothing from motion or physio data is stored in bids struct in the script.
That's strange. We have imported dozens of BIDS without electrodes.tsv - maybe it would throw an error when it does not find 10-20 electrodes? Do you have a dataset number so we can test?
Sure, it's a super minimal data set with only one participant. Let me know if you were able to replicate the error, otherwise I will double check my setup. An older version from a year ago or so used to work without problem https://drive.google.com/drive/folders/1NCVe64hndfL7C2iAaswpBzRM1S7WtEDG?usp=sharing
Thanks. Dung would you mind testing?
Thanks @sjeung for the code addition. Two general comments:
@arnodelorme This commit introduced the changes where electrodes.tsv and channels.tsv are expected to exist: https://github.com/sccn/bids-matlab-tools/commit/3b58ba60758f48f549e7b9bb37c417a38385747b#diff-d9ee0e01064e30c819fe9a2b79c08281ec47f91adf17b586b5e49d1b23af470d. This is why it's failing for Sein. Was there a reason for the change?
we fixed the error of not being able to import the BIDS dataset when no electrodes.tsv file is found in a58f356
Thanks @sjeung for the code addition. Two general comments:
- Can you point to the BIDS motion spec from which this implementation follows? How are scans.tsv, motion.tsv, and physio.tsv supposed to be handled?
- In order to avoid monolithic code, it would be better if you can refactor line 562-760 into its own function. It seems to me that there're only few variables from before that the section depend on.
@arnodelorme This commit introduced the changes where electrodes.tsv and channels.tsv are expected to exist: 3b58ba6#diff-d9ee0e01064e30c819fe9a2b79c08281ec47f91adf17b586b5e49d1b23af470d. This is why it's failing for Sein. Was there a reason for the change?
Hi Dung, thanks for the input! Here is the current spec for motion data.
https://docs.google.com/document/d/1iaaLKgWjK5pcISD1MVxHKexB3PZWfE2aAC5HF_pCZWo/edit?usp=sharing
and yes, I will get to it and push the changes in the next days.
Hello @dungscout96, I am going to work on this today and would like to check with you
1) whether you prefer it to be a subfunction inside pop_importbids.m or an extra mfile?
And
2) could you check whether you would then use this function for potentially more modalities (e.g., eyetracking)? Then I would name it something like "importbids_noneeg" or so (although MEG and iEEG are already covered in the main function).
Hi @sjeung, a subfunction as you are doing currently is good and make the extraction into external file easy if need to. I'd think this function will handle all modalities stored under */motion/ directories.
As discussed with @arnodelorme and @dungscout96, here is the pull request that implements read-in of BIDS-formatted motion and physio data following the current specification (extension proposal still in motion's case).
@dungscout96 I still have to run some tests for multi-run and multi-session cases. You can already go ahead and review if you want (about the general structure of the code .etc), and I would also share some test data sets in BIDS format so you can properly try this.