UMPsychMethodsCore / MethodsCore

All of the projects that the methods core develops, combined into one repository!
7 stars 0 forks source link

QualityControl / CheckMotion #373

Closed rcwelsh closed 8 years ago

rcwelsh commented 8 years ago

We need to move from using "size" to "numel". I put in a SubjDir entry of

'subject',2,[]

as the person has no runs, but I didn't want to omit from a long list.

line 25 of MotionSummary_mc_central.m uses size, and [] has size of 0 x 1

Not sure who will fix this?

@heffjos @mangstad ?

-Robert

rcwelsh commented 8 years ago

There are two lines:

for jRun = 1:size(SubjDir{iSubject,3},2)

should be

for jRun = 1:numel(SubjDir{iSubject,3})

rcwelsh commented 8 years ago

I've just noted that the fdOutliers.csv file appears to have one more row than number of time points in the files.

that doesn't seem right.

@heffjos @mangstad @dankessler @sripada

rcwelsh commented 8 years ago

I think the 1+ point is from using realign.dat -- which did not strip off the reference volume. in old fMRI stream there strategy was to pull volume #10 and add copy of it to the beginning of the time series, then tell mcflirt to align to volume 0. after realignment this dummy first image was stripped.

dankessler commented 8 years ago

Chatted about this briefly this AM, and @heffjos indicated he was going to take point on bugfixing

heffjos commented 8 years ago

For numel, what version of matlab are you using? I tested using '[]' for the run specification on both venus (matlab 2012a) and dysthymia (matlab 2012b) and everything ran without error. In both, I also get size([]) = [0 0]. This also complies with their documentation: http://www.mathworks.com/help/matlab/math/empty-matrices-scalars-and-vectors.html

For fdOutliers.csv, my output matched the number of volumes for the scan. If the 1+ point is caused by the realign.dat, would it be better to remove the point from the realign.dat so it is not propagated in further analyses (first level, conntool, etc.)?

mangstad commented 8 years ago

I can confirm the size thing as well, with Matlab 2007b, 2012b, and 2013a.

SubjDir = { ‘subject’,2,[] } size(SubjDir{1,3}) yields a size of [0 0]

And for fdOutliers thing, I agree with Joe that it would make more sense to strip the point from the realign.dat, rather than try to have MotionSummary guess whether you have an extra timepoint at the beginning or not.


Electronic Mail is not secure, may not be read every day, and should not be used for urgent or sensitive issues

rcwelsh commented 8 years ago

Okie, sorry for the delay. I'm using matlab 2012b on PANLAB and I do the following to populate SubjDir

%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% Experiment Directory %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%

Exp = '/net/data4/MSIT2005/';

%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% The list of subjects %%% col 1 = subject id as string, col 2 = subject id as number, col 3 = runs %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%

[subjects nruns nconn] = textread('subject_list_msit.csv','%s%d%d'); SubjDir = cell(numel(subjects),3);

for iS = 1:numel(subjects); SubjDir{iS,1} = subjects{iS}; SubjDir{iS,2} = iS; SubjDir{iS,3} = (1:nruns(iS))'; end

And the 120th row of the SubjDir is

SubjDir{120,:} 'kdf12ocdp0110' 120 Empty matrix: 0-by-1

So then if I do the size command:

size(SubjDir{120,3},2)

It returns:

1

Which breaks it.

size(SubjDir{120,3})

return

0 1

Bottom line, it's breaking in MATLAB 2012B and needs to be fixed. -Thanks.

rcwelsh commented 8 years ago

basically it's coming from how i create the run list by doing:

(1:nRuns(iS))'

and if nRuns(iS) happens to be zero that creates a

1 x 0 matrix and the transpose makes it a 0 x 1 matrix.

numel will resolve the issue.

mangstad commented 8 years ago

Yep. We never ran into that case because most users are not populating SubjDir in that way so it wasn't a consideration.

@heffjos can implement this fix on CheckMotion. Probably a good idea to do a find for size on the code to make sure it's not happening elsewhere (in this toolbox, though also in the broader scope it probably happens in other tools as well so we'll need to look around).

@rcwelsh do you have thoughts about our comments above regarding the extra timepoint from the old data with realign.dat? I don't really want to put some hacky contrived check in there to figure out if it's an old realign file or not.

heffjos commented 8 years ago

Maybe this should be labeled as an enhancement since SubjDir was not expected to be populated in such a way. I updated the code to use numel and is waiting to be merged into the beta tree along with other enhancements made to CheckMotion. The pull request is #361

dankessler commented 8 years ago

I'm going to close this issue. The fix isn't merged into public yet, but it has made its way into CheckMotion_beta, which will get pulled up to public shortly.