sccn / roiconnect

ROI connectivity analysis in EEG
39 stars 17 forks source link

Snippets dimensions and extra code #60

Closed 99KHayes closed 1 year ago

99KHayes commented 1 year ago

Snippets section of code in pop_roi_connect is still having some functionality issues.

I want to note that I do have the latest version on my computer as I downloaded it again after the issue #53 was fixed so it was downloaded on 7/19. I am not an EEG expert or even a MATLAB expert so its entirely possible I might be slightly wrong here. The bottom line is despite the snippets code update we are still unable to run snippets off the command line.

  1. Lines 264-274 are old code that was not commented out or deleted and having the double section under the line 250 for isnip = 1:nsnips is causing an error in indexing the snips from EEG.roi.source_roi_data on lines 264 and thus 267 as the 3rd dimension is the number of epochs in the above snip and not the whole EEG.roi.source_roi_data on the second loop.
    The fix: is to remove lines 264-274. I believe this is alluded to in issue #58 but also wanted to make sure it was known its actually preventing analysis without omitting those lines myself

  2. I noticed the same error I reported last time in issue #53 showing up in line 278 again as fc_name = options{2}{fc}; when it should be pulling from the g input parameters as before as fc_name = g.methods{fc}; This will avoid errors if there is more than one connectivity matrix being assessed The fix: change line 278 to be fc_name = g.methods{fc};

  3. Lines 277 through 295 I think are indented one two many times

    for fc = 1:n_conn_metrics
            fc_name = options{2}{fc};
            [first_dim, second_dim, third_dim] = size(conn_matrices_snips{1,fc});
    
            conn_cell = conn_matrices_snips(:,fc); % store all matrices of one metric in a cell
            mat = cell2mat(conn_cell);
            reshaped = reshape(mat, first_dim, nsnips, second_dim, third_dim);
            reshaped = squeeze(permute(reshaped, [2,1,3,4]));
            if strcmpi(g.fcsave_format, 'all_snips')
                EEG.roi.(fc_name) = reshaped;
            else
                if nsnips > 1
                    mean_conn = squeeze(mean(reshaped, 1)); 
                else
                    mean_conn = reshaped;
                end
                EEG.roi.(fc_name) = mean_conn; % store mean connectivity in EEG struct
            end
        end

    There is an error of Error using reshape: Number of elements must not change. Use [] as one of the size inputs to automatically calculate the appropriate size for that dimension. because of line 283 reshaped = reshape(mat, first_dim, nsnips, second_dim, third_dim);. This is happening because this cell block is indented under the original line 250 snip loop for isnip = 1:nsnips. So the conn_matrices_snips variable should be being built during the original for loop in line 257

    source_roi_data_save = EEG.roi.source_roi_data;
    for isnip = 1:nsnips
        roi_snip = source_roi_data_save(:,:,(isnip-1)* snip_eps + 1 : (isnip-1)* snip_eps + snip_eps); % cut source data into snippets
        EEG.roi.source_roi_data = single(roi_snip);
        EEG = roi_connect(EEG, 'morder', g.morder, 'naccu', g.naccu, 'methods', g.methods,'freqresolution', g.freqresolution, 'roi_selection', g.roi_selection); % compute connectivity over one snippet
        for fc = 1:n_conn_metrics 
            fc_name = g.methods{fc};
            fc_matrix = EEG.roi.(fc_name);
            conn_matrices_snips{isnip,fc} = fc_matrix; % store each connectivity metric for each snippet in separate structure
        end
    
        if strcmpi(g.firstsnippet, 'on')
            nsnips = 1;
        end

    and then that variable would have all of the snips stored separately. Instead because lines 277 through 295 are not indented they are attempting to process the code on an incomplete conn_matrices_snips and rearranging the dimensions of it with the total number of snips in the EEG and the dimensions on line 283 are off because of it. The fix: Unindent lines 277 through 295 one indent so they are the same indention level as the line 250 snip loop for isnip = 1:nsnips

nguyen-td commented 1 year ago

Hi, thanks for your report! I actually provided a fix in #59 from 7/21 which should hopefully fix the already known errors. Could you please pull the most updated version and let me know if you still run into these issues?

Thanks for reporting the second bug, I will look into it. Do you have some minimal sample code I could run to reproduce the error?

99KHayes commented 1 year ago

Hi, I saw that as soon as I pressed submit. That's what you get for starting something a few days and then coming back to it without refreshing. Yes issue #59 fixed the issues we were having and it now progresses pass those lines in the code. The second fix I suggested should probably still be implemented to prevent errors when people enter the parameter functions in atypical order.

I am unable to share any files at the moment because of the nature or our work, but I will speak to our techs and see if they can make me a fake file with all the right parameters. We reran our data on the newly updated code (downloaded 7/24 morning) because the old files that had the roi activity already ran did not have the proper variables in EE.roi (namely the EEG.roi.pnts was not a feature in the old one just EEG.pnts) I am getting an error trying to run it now at roi_activity on line 238

 frqs = sfreqs(fres, EEG.srate); 
Unrecognized function or variable 'sfreqs'.
Error in roi_activity (line 239)
frqs = sfreqs(fres, EEG.srate);

I haven't had too long to chase this error down as before in making these reports, so it might be on us, but I wanted to get you a quick answer. From quick glance it looks like it might be a toolbox dependency. Once I installed it it worked!

From our labs perspective we are now able to run snips for the connectivity, thank you for all your hard work!

nguyen-td commented 1 year ago

Your suggestions from #53 have also already been implemented, the order shouldn't matter now. For example: https://github.com/sccn/roiconnect/blob/b8dfd7698e6b9bdf3c1e689ac7bb72c7675c44ac/pop_roi_connect.m#L264

Yes, the sfreqs issue might have been related to a toolbox dependency. It's a function from the mvgc toolbox which is included in the libs folder.

Happy to hear that it works! If you think there is still something missing, please feel free to reopen this issue. Also, since it seems to be working for you, I think there is no need to share files anymore.