Back to the main page.
Bug 1201 - cluster randomization broken when 'freq_time', or 'chan_freq_time' with nchan = 1
| Status | CLOSED FIXED |
| Reported | 2011-11-30 12:22:00 +0100 |
| Modified | 2014-05-14 20:08:50 +0200 |
| Product: | FieldTrip |
| Component: | core |
| Version: | unspecified |
| Hardware: | All |
| Operating System: | All |
| Importance: | P3 blocker |
| Assigned to: | Robert Oostenveld |
| URL: | |
| Tags: | |
| Depends on: | |
| Blocks: | |
| See also: |
Roemer van der Meij - 2011-11-30 12:22:14 +0100
This used to work fine half a year ago. Now, in the case of 'chan_freq_time' with nchan = 1, a neighbors-structure is necessary. This cannot be constructed when there is only one channel, ft_prepare_neighbor throws a 'no neighbors found' error. Ft_freqstatistics used to be able to handle empty neighborhood structures. If I change the data to 'freq_time', ft_freqstatistics stills throws an error, it checks for the presence of a neighborhood structure. If this check is commented out, the function fails when selecting data, cause it still tries to select channels. What has happened here, is the dimord 'freq_time' no longer supported?
Jörn M. Horschig - 2011-11-30 14:26:01 +0100
hm, sounds like I introduced that one, although I don't know why it does not work when there are no neighbours. Just to be sure, if data dimord does not contain 'chan', no neighbours should be required, right? That's an easy fix (by extending the if-statement in line 107). About the other stuff, JM restrucuted ft_freqstatistics in May, revision 3499. Probably something went wrong there and this option is not fully supported anymore. I would propose you copy the exact error message here or fix it yourself ;) I am only doing channel-clustering at the moment, so I have no clue what might go wrong
Jörn M. Horschig - 2011-11-30 14:26:48 +0100
just to make it clear, I only introduced the necessity of neighbours, not that clusterstats on 'freq_time' does not work anymore
Roemer van der Meij - 2011-11-30 18:38:52 +0100
Nope, no neighbour-selection if 'freq_time' :). I'll try to post a proper error message later one, but 'freq_time' fails at the select data call (thought it was because of trying to select non-existing channels).
Boris Reuderink - 2011-12-06 12:56:22 +0100
I don't quite follow the discussion (requires much in-depth knowledge). Jörn, can I regard this bug as a defect by a developer, and assign it to you?
Jörn M. Horschig - 2011-12-07 09:30:23 +0100
sure, I could look into that, would give me some insight into how this function actually works ;) Though I guess it's really a stupid mistake
Jörn M. Horschig - 2011-12-07 09:36:00 +0100
Roemer, could you quickly post a snippet of code to run this the way you encountered the error? I'll then fix it before the FT meeting.
Roemer van der Meij - 2011-12-07 10:49:58 +0100
For the case of "freq_time" % make fake dataset freq = cell(1,10); for idat = 1:10 freq{idat}.label = {'singlechan'}; freq{idat}.dimord = 'freq_time'; freq{idat}.powspctrm = rand(10,30); freq{idat}.freq = 1:10; freq{idat}.time = 0.1:0.1:3; freq{idat}.cfg = []; end % do stats cfg = []; cfg.method = 'montecarlo'; cfg.statistic = 'depsamplesT'; cfg.alpha = 0.05; cfg.correctm = 'cluster'; cfg.clusterstatistic = 'maxsum'; cfg.clusterthreshold = 'parametric'; cfg.numrandomization = 500; cfg.design = [ones(1,5) ones(1,5).*2; 1:5 1:5;]; cfg.ivar = 1; cfg.uvar = 2; stat = ft_freqstatistics(cfg,freq{:}); *********************************** *********************************** *********************************** And for the case of 'chan_freq_time' with one channel % make fake dataset freq = cell(1,10); for idat = 1:10 freq{idat}.label = {'singlechan'}; freq{idat}.dimord = 'chan_freq_time'; freq{idat}.powspctrm = rand(1,10,30); freq{idat}.freq = 1:10; freq{idat}.time = 0.1:0.1:3; freq{idat}.cfg = []; end % do stats cfg = []; cfg.method = 'montecarlo'; cfg.statistic = 'depsamplesT'; cfg.alpha = 0.05; cfg.correctm = 'cluster'; cfg.clusterstatistic = 'maxsum'; cfg.clusterthreshold = 'parametric'; cfg.numrandomization = 500; cfg.design = [ones(1,5) ones(1,5).*2; 1:5 1:5;]; cfg.ivar = 1; cfg.uvar = 2; stat = ft_freqstatistics(cfg,freq{:});
Boris Reuderink - 2011-12-07 11:21:12 +0100
Assigned bug to Jörn (defect by developer).
Roemer van der Meij - 2011-12-07 12:43:12 +0100
Oh, BTW, the 'chan_freq_time' version also (most likely) suffer from the same bug if you have multiple channels, but have cfg.avgoverchan = 'yes' (no neighbours there either).
Jörn M. Horschig - 2011-12-09 14:22:01 +0100
Are you sure that freq_time worked in an earlier version? I cannot find any change that might have caused this to not work anymore. statistics_montecarlo and findclusters have not changed since a long time I kinda hacked my way through chan_freq_time now, but the problem is that findcluster explicitly expects a 3D matrix. I could add a dummy dimension to make it work - but before I do that I need feedback from codemaster RO ;) btw chan_freq_time should work now. I don't like the change though, but I couldn't think of a better way to include this. Could you check whether it works for you as well?
Roemer van der Meij - 2011-12-09 14:34:02 +0100
I don't remember when it did actually. It worked fine when it was implicit though (only 1 channel in chan_freq_time, and no neighbours), which would be the same if cfg.avgoverchan = yes I think. All of the situations should be handled by the same code I assume. I assume it's just the requirement of neighbours when no neighbours are there.
Jörn M. Horschig - 2011-12-09 14:57:36 +0100
oh, the freq_time case has nothing to do with neighbours - the code crashes in findclusters because it expects a 4D matrix, but without channels it is 3D (freq_time_rpt) Any chan_freq_time case should work again (neighbours are only required if nchannels > 1 or ~avgoverchan)
Roemer van der Meij - 2011-12-09 15:16:56 +0100
Hmmm, are you sure you committed? I just ran the test code and it still fails in clusterstat.m when it requires the neighbour-struct at line 197
Jörn M. Horschig - 2011-12-09 15:47:55 +0100
oh right, my fault, I also changed that function actually please try again in a few minutes ;)
Jörn M. Horschig - 2012-01-06 15:52:20 +0100
Roemer, does it work for you?
Roemer van der Meij - 2012-01-09 10:54:26 +0100
If you run the first piece of test code, there's an error. The second piece of test code works fine. (see previous comment)
Jörn M. Horschig - 2012-01-09 11:32:57 +0100
hm strange, back then I was so sure that it should work... but I had problems with my repository anyway, so maybe committing didn't work or something. Anyway, for now I decided to add a dummy channel dimension in case the channel dimension is absent. 60 $ svn ci ft_freqstatistics.m -m "bugfix-#1201- freqstatistics can handle single channel data wo channel dimension (again)" Sending ft_freqstatistics.m Transmitting file data . Committed revision 5099. And I commited a testscript using your two example scripts (which runs fine now) 63 $ svn add test_ft_freqstatistics.m A test_ft_freqstatistics.m jorhor@mentat301:~/FieldTrip/trunk/test 64 $ svn ci test_ft_freqstatistics.m -m "enhancement - testscript for freqstatistics" Adding test_ft_freqstatistics.m Transmitting file data . Committed revision 5100.
Jörn M. Horschig - 2012-08-23 14:02:08 +0200
bug closing time (http://www.youtube.com/watch?v=xGytDsqkQY8)
Robert Oostenveld - 2014-03-22 09:42:29 +0100
around http://code.google.com/p/fieldtrip/source/detail?r=9306 (might be one earlier or later) the test script test_ft_freqstatistics started failing. Turns out it is due to a freq structure with dimord=freq_time, which is apparently the topic of this bug. I am not sure whether the data structure in the test script should work. There is a discrepancy: freq = cell(1,10); for idat = 1:10 freq{idat}.label = {'singlechan'}; %%%% here freq{idat}.dimord = 'freq_time'; %%%% and here freq{idat}.powspctrm = rand(10,30); freq{idat}.freq = 1:10; freq{idat}.time = 0.1:0.1:3; freq{idat}.cfg = []; end If I remove the label field, the test fails because it is not freq data. If I change the dimord into chan_freq_time, it all works fine. @Roemer: Why is this test script supposed to work on this data?
Robert Oostenveld - 2014-03-24 08:54:59 +0100
(In reply to Robert Oostenveld from comment #19) I looked at the discussion thread of this bug. There seems no clear reason why dimord freq_time without chan should be supported. It is not a freq structure according to the definition in the help of ft_datatype_freq and it will also not be supported by most other functions. Therefore I will remove support for freq_time from the test script. roboos@mbp> svn commit Sending test_ft_freqstatistics.m Committed revision 9309.
Roemer van der Meij - 2014-03-24 14:27:19 +0100
(In reply to Robert Oostenveld from comment #19) I encountered a situation at the time, I think on the mailinglist or via a friend, where they wanted to do ft_freqstatistics with cfg.avgoverchan = 'yes'. I can't remember why dimord = 'freq_time' made sense at the time. I think, from the current perspective, cfg.avgoverchan = yes would lead to a powspctrm of (1,x,y), and as such the dimord would be 'chan_freq_time'. Removing 'freq_time' from the test script makes sense. This reminds me of our existential dimord-debate a number of weeks back ;)