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

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

Open
jutako opened this issue Jun 7, 2018 · 3 comments
Open

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

jutako opened this issue Jun 7, 2018 · 3 comments
Assignees
Labels

Comments

@jutako
Copy link
Collaborator

jutako commented Jun 7, 2018

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:

  • keep the current logic and check that all functions that use structconv.m operate as expected (using cell2mat.m to convert a cell array to a matrix if needed)
  • revert the change and the one fix I already made

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.

@jutako jutako added the bug label Jun 7, 2018
@zenBen
Copy link
Member

zenBen commented Jun 10, 2018

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
Copy link
Member

zenBen commented Aug 3, 2018

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:

  • data has numeric event type field
  • data is not loaded with CTAP_load_data() (which converts event.type field to char as a precaution)

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
Copy link
Member

zenBen commented Aug 3, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants