Back to the main page.
Bug 2785 - ft_timelockgrandaverage selects wrong time window and thus returns wrong results
| Status | CLOSED FIXED |
| Reported | 2014-12-16 11:53:00 +0100 |
| Modified | 2019-08-10 12:29:30 +0200 |
| Product: | FieldTrip |
| Component: | core |
| Version: | unspecified |
| Hardware: | All |
| Operating System: | All |
| Importance: | P5 critical |
| Assigned to: | |
| URL: | |
| Tags: | |
| Depends on: | |
| Blocks: | |
| See also: |
Thomas Hartmann - 2014-12-16 11:53:21 +0100
i found a critical bug in ft_timelockgrandaverage. basically, if the cfg.latency field is used and the dof field is present in the data, data is always selected from the beginning of the trials. the problem is in line 154ff: if hastime timesel = nearest(varargin{i}.time, cfg.latency(1)):nearest(varargin{i}.time, cfg.latency(2)); varargin{i}.time = varargin{i}.time(timesel); end if hasdof timesel = nearest(varargin{i}.time, cfg.latency(1)):nearest(varargin{i}.time, cfg.latency(2)); varargin{i}.dof = varargin{i}.dof(chansel,timesel); end in line 156, the original time field is modified according to the latency. in line 160, this altered field is used for a new time selection. this leads to a mismatch because the time field is already cut while the rest of the data is not. i would suggest using the well tested ft_selectdata function instead. i would also be willing to fix the bug using ft_selectdata if you think it is the right way to go. please let me know as soon as possible because this bug leads to wrong results and should be fixed asap. best, thomas
Robert Oostenveld - 2014-12-16 13:43:19 +0100
I can confirm the problem and wrote a test script for it (which is presently failing with an error). mac011> svn commit test/test_bug2785.m Adding test/test_bug2785.m Transmitting file data . Committed revision 10053. I agree that ft_selectdata should be used for a consistent selection of all fields of the input data. Please go ahead and improve the code. You might want to review/rerun the relevant test scripts during the rewrite to make sure that existing functionality remains intact. mac011> grep -l ft_timelockgrandaverage fieldtrip/test/test*.m test_bug1162.m test_bug2372.m test_bug2471.m test_bug2785.m test_ft_timelockgrandaverage.m test_tutorial_eventrelatedaveraging20130308.m
Thomas Hartmann - 2014-12-16 13:56:17 +0100
(In reply to Robert Oostenveld from comment #1) test_bug2785.m fails where it should not: Index exceeds matrix dimensions. Error in ft_timelockanalysis (line 278) s (:,windowsel) = s (:,windowsel) + dat; % compute the sum Error in test_bug2785 (line 30) avg = ft_timelockanalysis(cfg, data); also, doing some of the other tests will prove to be quite tedious because the path to the data is hardcoded. i will do my best, though...
Robert Oostenveld - 2014-12-16 14:02:31 +0100
(In reply to Thomas Hartmann from comment #2) sorry, that was something I fixed on the fly, but forgot to commit. It had to do with estimating sample from the first trial, which has length 1. mac011> svn commit utilities/ft_datatype_raw.m ft_timelockanalysis.m Sending ft_timelockanalysis.m Sending utilities/ft_datatype_raw.m Transmitting file data .. Committed revision 10054. regarding the other tests: if they involve data that is on disk, then you don't have to bother. I was hoping that there would be some test scripts that start with some data that is generated on the fly.
Robert Oostenveld - 2014-12-16 16:09:38 +0100
note that I just did a small update regarding an apparent typo in an if-clause mac011> svn commit ft_timelockgrandaverage.m Sending ft_timelockgrandaverage.m Transmitting file data . Committed revision 10056.
Thomas Hartmann - 2014-12-17 11:05:46 +0100
alright, here is the pull request.
Robert Oostenveld - 2014-12-17 16:04:49 +0100
after some discussion on https://github.com/fieldtrip/fieldtrip/pull/52#issuecomment-67329094 I updated the code. mac011> svn commit ft_timelockgrandaverage.m test/test_tutorial_eventrelatedaveraging20130308.m test/test_bug2785.m Sending ft_timelockgrandaverage.m Sending test/test_bug2785.m Sending test/test_tutorial_eventrelatedaveraging20130308.m Transmitting file data ... Committed revision 10059. this works with all current test scripts.