hipercog / ctap

Computational Testing for Automated Preprocessing - a Matlab toolbox extending EEGLAB functionality for batch processing of EEG
Other
22 stars 10 forks source link

Output of structconv.m changed -> need to review usage #24

Open jutako opened 6 years ago

jutako commented 6 years ago

The output of structconv.m was changed (last fall) such that all output arrays are cell arrays, even for numeric values, for which a matrix would be a more natural format to work on. I have thus far fixed one problem that arose because of this change, but I suspect there will be more. Luckily the change will cause errors to be thrown as the permitted operations on matrices and cell arrays differ considerably.

We have two options:

If there is a native Matlab function or short expression to do the conversion without structconv.m, I would also be happy to ditch the whole function.

zenBen commented 6 years ago

Sorry for introducing this bug, it was in response to a tricky issue with events: Mixed-type Events Issue

Clearly, the event.type field needs to be of a single data type. I don't recall exactly but probably I changed structconv.m in order to facilitate that. I suggest looking at the option of reverting structconv.m and then editing eeglab_merge_event_tables.m (which calls structconv()) to ensure event.type fields are all the same data-type. pop_epoch() could be used as a test unit (since it is the bottleneck for using mixed event.types).

zenBen commented 6 years ago

Update: on reflection, the reason for my change to structconv() is that I wanted to process structs with fields containing mixed data (chars and numbers, representing different trigger types), yet I didn't see the restriction in the function docs: "All data in one field is of the same type"

Following your second option above, I now reverted structconv(). I cannot revert the fix you made since I don't know where it is.

However, this will mean that (because key CTAP functions such as CTAP_peek_data() are adding events with char event.type values) any pipe will break where following conditions hold:

The following lines exist in CTAP that pass (probable) event structures to structconv(). There are so many uses that it seems mandatory to fix the event.type field datatype to be uniform on data loading.

./ctap/src/utils/convert/recordingev2eeglabev.m 73  EventEeglab = structconv(EventEeglab);
./ctap/src/utils/convert/recordingev2eeglabev.m 87  EventEeglab = structconv(EventEeglab);
./ctap/src/utils/eeglabutils/events/eeglab_merge_event_tables.m 143 [event1_cell, labels1] = struct_to_cell(structconv(event1)); %convert to cell
./ctap/src/utils/eeglabutils/events/eeglab_merge_event_tables.m 165 [event2_cell, labels2] = struct_to_cell(structconv(event2)); %convert to cell
./ctap/src/utils/eeglabutils/events/erp_evmod/eeg_checkevent.m  90  EEG.event = structconv(EEG.event);
./ctap/src/utils/eeglabutils/events/erp_evmod/eeg_checkevent.m  92  EEG.event = structconv(EEG.event);
./ctap/src/utils/eeglabutils/events/erp_evmod/eeg_checkevent.m  106 EEG.event = structconv(EEG.event);
./ctap/src/utils/eeglabutils/events/erp_evmod/eeg_checkevent.m  124 EEG.event = structconv(EEG.event); %Convert back to element organization
./ctap/src/utils/eeglabutils/events/erp_evmod/mark_resp2stim.m  129 EventMod = structconv(EEG.event); %to plane organization
./ctap/src/utils/eeglabutils/events/erp_evmod/mark_resp2stim.m  275 EEG.event = structconv(EventMod);

HOWEVER, no one is forced to use CTAP_load_data() (e.g. it seems Seamless doesn't use it). Any suggestions for a solution to ensure good functioning of event-related code??

zenBen commented 6 years ago

Added the following to the beginning of eeglab_merge_event_tables() to help ensure event.type field remains uniform.

%% Check and ensure type field datatype is uniform
for idx = 1:numel(event1) 
    if isnumeric(event1(idx).type) 
        event1(idx).type = num2str(event1(idx).type);
    end
end
for idx = 1:numel(event2) 
    if isnumeric(event2(idx).type) 
        event2(idx).type = num2str(event2(idx).type);
    end
end