sccn / EEG-BIDS

23 stars 17 forks source link

Custom fields for events #5

Closed arnodelorme closed 3 years ago

arnodelorme commented 4 years ago

Hi Agatha,

I have looked at the code and indeed the eInfo field is not being used.

Would you mind to share some data and your script and we will look at it right away. We actually need to implement that feature for HED support.

Cheers,

Arno

On Nov 2, 2019, at 9:55 PM, Scott Makeig smakeig@ucsd.edu wrote:

Hi all ~ I posted the following on the GitHub for bids-export.m tools. There’s an issue with bids_export.m in that it fails to actually implement what’s promised for the eInfo input variable in the help. Looking through the code, there’s a hardcoded routine to write pre-defined EEG.event fields as columns, and eInfo EEG.event fields are not considered. Sorry if this is in the wrong place. I wasn’t sure if to post this on bugzilla or here or GitHub! Agatha

alenarto commented 4 years ago

Hi Dung. There's an issue in revised code.

  1. First error is as below. It seems to occur because there is an expectation that the user is using pinfo (otherwise "participants" variable is not created). Since I don't use bids_export.m to create my participants.tsv - this occurs.

Undefined function or variable 'participants'.

Error in bids_export (line 398) subjectStr = participants{iSubj+1,1}; % first row of participants contains header

  1. If I comment out above line and instead use the original code that assigns for-looop index to subjectStr, I still get an error as below. I did not debug this at this point, perhaps it's related to above (since not string).

First argument must be a string array or cell array of character vectors. Error in bids_export>copy_data_bids (line 594) tagString = join([userTags, hedTags],',');

Error in bids_export (line 406) copy_data_bids( files(iSubj).file{1}, fileOut, opt, chanlocs{iSubj}, opt.copydata);

Error in bids_write (line 136) bids_export(data,'targetdir','./bidsexport','taskName',params.task,'gInfo',generalInfo,'tInfo', tInfo,'cInfo',cInfo,'eInfo',eInfo,'eInfoDesc',eInfoDesc, 'README', README,'chanlocs',params.chanlocs,'copydata',1);

dungscout96 commented 4 years ago

Hi Agatha, I updated the code to fix the first bug you reported. I'm not sure why you're running into the second bug but I did make some changes related to that section of the code for other purpose. Can you see if that happens to fix your error as well? If not can you provide the dataset in which the tool is failing? (I tried with the dataset you provided and it seems fine)

cll008 commented 3 years ago

Hi @alenarto, this is Clement. I wanted to follow up here and see if Dung's fixed worked for you or if we can take a look at your script and datasets to improve our tools' functionality. Thanks.

alenarto commented 3 years ago

@cll008 @dungscout96 Hi guys - finally got to this. So several comments on using v5.3 of bids_export.m.

  1. There's a change that the code now requires tInfo.HardwareFilters to be a struct. This is not documented anywhere and resulted in an error for me as below. There was also no demo in the example files of how this is supposed to be written so I had to play around until I figured it out. Guidance would be helpful for the user.

E_rror using bids_export>checkfields (line 1058) Parameter tInfo.HardwareFilters must be a struct

Error in bids_export>copy_data_bids (line 1023) tInfo = checkfields(tInfo, tInfoFields, 'tInfo');

Error in bids_export (line 571) copy_databids( files(iSubj).file{1}, fileOut, files(iSubj).notes{1}, opt, files(iSubj).chanlocs{1}, opt.copydata);

  1. Regarding the creation of the *events.tsv file. First issue is that Dung's new code requires a change in the orientation of the cell containing the event name/field pairs {'name1' 'name2'; 'field1' 'field2'} becomes {'name1' 'field1'; 'name2' 'field2'}. This change is not documented. Second issue is that the code promises to automatically find and write the onset, duration, value fields, but it does not do so. On my first go I got the following output after only specifying my unique fields and assuming the code would do the rest. All 'n/a' values for the default fields.

_% 'eInfo' - [cell] additional event information columns and their corresponding % event fields in the EEGLAB event structure. Note that % EEGLAB event latency, duration, and type are inserted % automatically as columns "onset" (latency in sec), "duration" % (duration in sec), "value" (EEGLAB event type). For example % { 'HED' 'usertag'; % 'value' 'type' } % See also 'trial_type parameter.

onset duration sample trial_type response_time stimfile value onsetRelToMRIstart MRlatency n/a n/a n/a n/a n/a n/a n/a n/a n/a n/a n/a n/a n/a n/a n/a n/a 30sec block n/a n/a n/a n/a n/a n/a n/a n/a 2sec epoch n/a n/a n/a n/a n/a n/a n/a n/a 2sec epoch

I had to explicitly specify the onset /duration fields to get them to be written out:

_% original format % eInfo = {'onsetRelToMRIstart' 'eventType';... % 'MRlatency' 'stimtype'};

%new format eInfo = {'onset' 'duration' 'value' 'onsetsamples' 'onsetRelToMRIstart' 'eventType'; ... 'latency' 'duration' 'type' 'latency' 'MRlatency' 'stimtype'}; eInfo = eInfo'; %to accommodate Dung Truong changes_

With all this fixed - I think it's working. I'll report back if we catch issues. I'm not sure what the new auto inserted fields are (sample, trial_type, response_time, stim_file) but I assume that's bids. I basically ignore them - they end up being 'n/a's in the tsv file. The code does not automatically find them and save them if that's what it's supposed to be doing, so I save trial_type myself using the 'value' heading. It's a bit messy in the end result.

Best Agatha

cll008 commented 3 years ago

Thanks Agatha for testing and Young for 33aed826. eInfo should be properly implemented and event fields that can be filled automatically with EEG fields will be.

It looks to me though according to the code (and my previous usage) that the eInfo structure should be tall (n x 2) instead of long (2 x n). So the current supported format is the same as @alenarto's "old format." I might be wrong, @dungscout96 could you confirm and close the issue?

alenarto commented 3 years ago

@cll008 Clement - the "old format" was wide I believe. So I think we're agreed. My old code created wide cells, and I had to transpose them to make the code work (now {key1 val1; key2 val2}, previously {key1 key2 key3 etc; val1 val2 val2}). Worth double checking.

cll008 commented 3 years ago

Ah okay I see the eInfo = eInfo' now. Thanks for clarifying. New format is the same as how I've been using bids_export and is the same as how I read the code.