Back to the main page.
Bug 2471 - ft_timelockgrandaverage does not handle differences in trial length under 'within'
| Status | ASSIGNED |
| Reported | 2014-02-11 17:31:00 +0100 |
| Modified | 2014-03-13 10:25:11 +0100 |
| Product: | FieldTrip |
| Component: | core |
| Version: | unspecified |
| Hardware: | Macintosh |
| Operating System: | Mac OS |
| Importance: | P5 normal |
| Assigned to: | Diego Lozano Soldevilla |
| URL: | |
| Tags: | |
| Depends on: | |
| Blocks: | |
| See also: |
Alik Widge - 2014-02-11 17:31:04 +0100
I have encountered a behavior with ft_timelockgrandaverage that seems to me to be a bug. I'm using the attached .mat, and you can replicate the behavior with: cfg = []; cfg.channel = 'all'; cfg.keepindividual = 'no'; cfg.method = 'within'; out_data = ft_timelockgrandaverage(cfg,Dstruct.on.MSITOnset.global.x{:}); This will crash due to an array dimension mismatch. It does so because my different subjects/blocks in x{:} have different numbers of samples. Up around line 147, the ft_ function goes through varargin{:} and cuts down the averaged variable (in my case, 'avg') so that it has the same number of samples in each of the varargin{:} data structures. The thing that I feel is a bug: it cuts the data, (expressed as varargin{i}.(cfg.parameter) ), but it does NOT similarly cut the variance ( varargin{i}.var ) or the per-timepoint DOF ( varargin{i}.dof ). As a result, when it gets to line 193 and tries to do avgmat(s, :, :) = varargin{s}.(cfg.parameter).*varargin{s}.dof; the (cfg.parameter) and dof matrices are now of different sizes, and we crash. ---- I have fixed it in my local copy by adding, around line 150: case 'chan_time' varargin{i}.(cfg.parameter) = varargin{i}.(cfg.parameter)(chansel,timesel); % ALIK CODE HACK % if strcmp(cfg.method,'within') for fld = {'var','dof'} fld = fld{1}; if isfield(varargin{i},fld) varargin{i}.(fld)= varargin{i}.(fld)(chansel,timesel); end end end % END ALIK CODE HACK % I am not sure whether this would also apply to some of the other 'dimord' cases, as I have not carefully thought about them and don't have test data.
Alik Widge - 2014-02-11 17:34:26 +0100
https://www.dropbox.com/s/kczrroxel4tza80/reproduce_maybe_bug.mat as it is a >10MB file.
Diego Lozano Soldevilla - 2014-02-14 17:26:28 +0100
(In reply to Alik Widge from comment #1) Bug fixed by updating the 'dof' field size accordingly when different data lengths need to be averaged. Sending ft_timelockgrandaverage.m Transmitting file data . Committed revision 9204. and its test function Adding test_bug2471.m Transmitting file data . Committed revision 9205. However, I think we need to improve the data checking to avoid these matrix crashes. ft_timelockgrandmean (and ft_freqgrandmean) don't use ft_selectdata to ensure chan, time, freq... dimensions among varargin inputs are equal. I see potential solutions but not sure what to choose: a) ft_selectdata: after usual cfg checks, call ft_selectdata to ensure channel, time, frequency (for ft_freqgrandmean). Caveat: if something is wrong with ft_selectdata (old to new transition), we'll crash b) use private functions checkchan/time/freq to ensure proper matrix dimension. Not sure if grad/elec should be checked because in line 249 revision 9204 (ft_timelockgrandaverage) the varargin{1}.grad/elec is taken as output. I think field var should be deleted explicitly with 'within' method
Alik Widge - 2014-02-14 20:19:59 +0100
I am not actually sure that I would want you to do that consistency check. In fact, I've actively modified local copies of the FT functions to NOT do it, or at least to behave well when consistency is violated. Here's the situation I routinely encounter, and it seems to me that this *should* be common for others based on my limited experience: I run N subjects. Of them, perhaps N/2 have one of many EEG channels missing, but usually a different one per subject (and there's a limit to how much I trust ft_channelrepair). Then, a semi-overlapping N/2 have trials where I just couldn't get rid of all the artifact, and I had to NaN out small chunks of data. Often, there's enough otherwise-good data in those trials and N is small enough that I do not wish to just throw out everything but the purest/cleanest of trials and channels. The behavior *I* consider reasonable is for the functions to gracefully use all the data available at any given chan x time or chan x freq x time point, keeping track of DOF appropriately, but basically behaving analogous to matlab's builtin nanmean(). It requires a bit of bookkeeping about which 2D or 3D points exist for each of the varargin{}, but in the relative scheme of things, that's not a big amount of memory.
Diego Lozano Soldevilla - 2014-02-16 13:50:17 +0100
(In reply to Alik Widge from comment #3) Dear Alik, Thank you for your comments and suggestions: we'll take into account. Lasts comments I made were to inform the fieldtrip development team to help me out the best way to proceed. Regarding how to take into account different missing channels and what to do with them, it might be a different problem but I'll consider it too Diego
Jan-Mathijs Schoffelen - 2014-02-26 14:31:50 +0100
Discussed in FT-meeting 26 Feb. It sparked a lengthy fundamental discussion about the representation of the data, potential ambiguities and consistency across the code base. Bottom line: we will revamp the ft_timelockgrandaverage function to behave consistently with respect to other XXXgrandaverage functions, or more in general, to functions that take multiple data arguments in the input. In other words, use ft_selectdata. We need a test-script with well-defined data designed to fail on situations where we want it to fail (with an explicit error generated in FT-code) and to succeed in situations where it should work. With respect to the missing channel issue. This is a different issue an can be followed up on bug 2221
Robert Oostenveld - 2014-03-13 10:16:51 +0100
the commit from Diego that is here http://code.google.com/p/fieldtrip/source/diff?spec=svn9285&r=9285&format=side&path=/trunk/ft_timelockgrandaverage.m causes the test_bug1162 script to fail. That test script calls ft_timelockgrandaverage for an unrelated reason (provenance of two input args), but uses a timelock representation that probably should be considered incorrect: timelock1 = []; timelock1.label = {'1' '2'}; timelock1.time = 1:5; timelock1.dimord = 'rpt_chan_time'; timelock1.avg = randn(2,5); timelock1.cfg = 'this is number 1'; As you see timelock1.avg has size [5 2], whereas based on the dimord it is expected to be [1 5 2]. I consider this to be an error in the test data, which I will fix by changing it into timelock1 = []; timelock1.label = {'1' '2'}; timelock1.time = 1:5; timelock1.dimord = 'rpt_chan_time'; timelock1.avg = randn(1,2,5); timelock1.cfg = 'this is number 1'; However, this only causes ft_timelockgrandaverage to fail further down.