Back to the main page.
Bug 2782 - ft_selectdata doesn't keep .trialinfo correctly if 'rpt_chan_time' but with only 1 trial
| Status | CLOSED FIXED |
| Reported | 2014-12-15 17:15:00 +0100 |
| Modified | 2019-08-10 12:30:56 +0200 |
| Product: | FieldTrip |
| Component: | core |
| Version: | unspecified |
| Hardware: | PC |
| Operating System: | Windows |
| Importance: | P5 normal |
| Assigned to: | Eelke Spaak |
| URL: | |
| Tags: | |
| Depends on: | |
| Blocks: | |
| See also: | http://bugzilla.fcdonders.nl/show_bug.cgi?id=2783http://bugzilla.fieldtriptoolbox.org/show_bug.cgi?id=3005 |
Johanna - 2014-12-15 17:15:54 +0100
Hello, I have a timelock structure that has dimord of 'rpt_chan_time' but happens to have only 1 trial (after others were rejected). This 'timelock' has .trialinfo of size [1x11]. Then the following: cfg=[]; cfg.trials=1; tmp=ft_selectdata(cfg,timelock) runs without error but produces tmp.trialinfo of size [1x1]. (I realise this is a weird scenario but it's a particular instance of a general piece of code that works fine for greater number of existing trials in the timelock structure). I traced the problem to line 1206: of ft_selectdata>makeselection>cellmatselect with the 'isvector'. Unfortunately at this low-level subfunction, the data structure is no longer around to test the size of data.trial, but ideally the seldim should be fixed at this point and forced to always use x = x(selindx,:,:,:,:,:); for the case of seldim==1. If the seldim needed still changing (or the input 'x' transposing) that should be done at a higher level where more information about the data structure is still present. It's easy to comment out the 'isvector', but I don't know what consequences it would have elsewhere or what situations the 'x' should be transposed prior to putting into 'cellmatselect'. Or.... 'wontfix' this bug, as I (the user) can simply make sure my code doesn't try to call ft_selectdata for only 1 trial with cfg.trials=1 ?? Thank you, Johanna
Robert Oostenveld - 2014-12-16 17:35:42 +0100
Hi Johanna, I can conform your issue with timelock = []; timelock.label = {'1', '2', '3'}; timelock.time = (1:1000)/1000; timelock.avg = randn(3,1000); timelock.trial = randn(1,3,1000); timelock.dimord = 'rpt_chan_time'; timelock.trialinfo = 1:11; Your usage is technically correct. The isvector serves to deal with the (sometimes) ambiguous row/column representation. If in that cellmatselect code the selindx would be a boolean vector rather than a list of indices, I think it would be possible to distinguish the transposed case correctly.
Robert Oostenveld - 2014-12-16 17:36:45 +0100
(In reply to Robert Oostenveld from comment #1) mac011> svn commit test_bug2782.m Adding test_bug2782.m Transmitting file data . Committed revision 10057.
Eelke Spaak - 2015-01-27 14:59:58 +0100
(In reply to Robert Oostenveld from comment #1) Hmm, this one is tricky. Robert, I don't think your idea of using a boolean index vector rather than numeric indices would work. The whole point of the isvector()-check is that we don't want to rely on the dimensionality of the input data at that point in the code (i.e. we don't trust an 1xN matrix and seldim==1 to actually be interpreted as an instruction to select along the first dimension). So even if we create an 1xN or Nx1 boolean index vector, we would still face the same problem (as we might consider transposing it in that case anyway). The 'clean' solution (i.e. most in line with cellmatselect()'s contract) would be to remove the isvector()-check, but as you say this probably breaks other things. I imagine those other things are not easily fixable elsewhere, right? (.freq and .time being row vectors comes to mind as a possible problem.)
Robert Oostenveld - 2015-01-27 15:12:29 +0100
(In reply to Eelke Spaak from comment #3) should the isvector not only apply in case it can unambiguously decipher the structure of the data? E.g. if seldim = 1 selindx = [1 2] and the data is 1x4 rather than 4x1, then it makes sense to transpose. In the other case the selection would error. Using logical indexing, the same selection would look like seldim = 1 selindx = [1 1 0 0] % but then named selbool again with the data being 1x4 or 4x1 But with logical indexing you could do if seldim==1 && numel(selbool)==size(data,1) % select along 1st dimension elseif seldim==1 && numel(selbool)==size(data,2) % select along 2nd dimension else ... The selbool should always be a vector and the number of elements should always be consistent with the size of the dimension that it is supposed to select from.
Eelke Spaak - 2015-01-27 15:25:56 +0100
(In reply to Robert Oostenveld from comment #4) Yes, that does make sense; however, we still have to create the selbool first. If we do: selbool = false(size(x, seldim)); selbool(selindx) = true; that won't work, as we run into the exact same issue as before.
Robert Oostenveld - 2015-01-27 16:28:56 +0100
(In reply to Eelke Spaak from comment #5) the selbool should be made elsewhere in the code. Tracing them back, line 255 and further seem relevant % trim the selection to all inputs, rpt and rpttap are dealt with later if hasspike, [selspike, cfg] = getselection_spike (cfg, varargin{:}); end if haspos, [selpos, cfg] = getselection_pos (cfg, varargin{:}, cfg.tolerance, cfg.select); end if haschan, [selchan, cfg] = getselection_chan (cfg, varargin{:}, cfg.select); end if haschancmb, [selchancmb, cfg] = getselection_chancmb(cfg, varargin{:}, cfg.select); end if hasfreq, [selfreq, cfg] = getselection_freq (cfg, varargin{:}, cfg.tolerance, cfg.select); end if hastime, [seltime, cfg] = getselection_time (cfg, varargin{:}, cfg.tolerance, cfg.select); end I am getting a bit worried about the impact of the change. I noticed that nans and [] also have a special meaning in the selection. I think that it is safer to take the alternative route, i.e. only take the transpose selection if that makes more sense.
- 2015-07-02 12:45:43 +0200
(In reply to Johanna from comment #0) Dear fieldtrip developers, I am facing a "problem" that seems to be very much related to this bug. The following code for selecting one trial and a particular latency runs without errors or warnings (I am using the most recent version of ft): cfg=[]; cfg.trials=[5]; cfg.latency=[2 50]; pp_data=ft_selectdata(cfg,pp_data); The trial selection works but the time selection fails. The returned latency equals 10s, the length of the shortest trial (although trial 5 actually contains more than 60s). This is done by the follwoing line in ft_selectdata: 267: if hastime, [seltime, cfg] = getselection_time (cfg, varargin{:}, cfg.tolerance, cfg.select); end I hope that this is really a bug and that my way of reporting is appropriate. The issue is easy to circumvent by the user by selecting trial and latency in separate calls; but it may cause confusion when overlooked. Kind regards, Jan
Teresa Madsen - 2015-08-13 19:54:01 +0200
I've encountered the same bug: when I run > data = ft_rejectvisual(cfg,data); on continuous data (1 long trial) to invalidate noisy channels, the field data.sampleinfo goes from 1x2 to 1x1, which leads to later errors. I tracked the problem down to line 1213 of ft_selectdata, within the subfunction cellmatselect, as I see you've already figured out from this prior bug report. Have you settled on a solution? Could there just be a check somewhere earlier in the code for single trial data? Thanks, Teresa
Teresa Madsen - 2015-08-14 21:55:54 +0200
(In reply to Teresa Madsen from comment #8) Whoops, sorry - I just downloaded and tried the latest version of FieldTrip, and the bug seems to have been fixed. Sorry for not trying that first! Thanks, Teresa
Teresa Madsen - 2015-11-17 20:10:48 +0100
(In reply to Teresa Madsen from comment #9) Nevermind, the error has re-emerged. I'm getting the same behavior as before, so I don't know if I changed something in my local code that then got overwritten when I updated to the latest version or what, but here's what's happening: I have a data structure with data.sampleinfo = [11,4696690], but after running... cfg = []; cfg.method = 'trial'; data = ft_rejectvisual(cfg,data); ...without invalidating anything, the resulting data structure has data.sampleinfo = 11, which causes later errors. The problem seems to occur at about line 1247 of ft_selectdata (in fieldtrip-20151112), where trial 1 is meant to be selected, but only the first value is selected because the code assumes row and column vectors to be equivalent: switch seldim case 1 if isvector(x) % sometimes the data is 1xN, whereas the dimord describes only the first dimension % in this case a row and column vector can be interpreted as equivalent x = x(selindx); else x = x(selindx,:,:,:,:,:); end Could an earlier check be made to identify single trial or continuous data that would disable that bit? For example, in the main ft_selectdata function, set... iscontinuous = numel(cfg.trials) == 1; ...and feed that into the subfunctions (makeselection and cellmatselect), so you can change line 1244 to... if isvector(x) && ~iscontinuous Would that work? Thanks, Teresa
Robert Oostenveld - 2015-11-18 09:30:34 +0100
(In reply to Teresa Madsen from comment #10) I reviewed the code once more. It is indeed due to swapping row and column by default, whereas in this case they should not be swapped. Swapping a vector is fine if the singleton dimension does not represent anything, but it should be avoided in case the singleton dimension is explicitly known. As Johanna mentioned earlier, the problem is that cellmatselect lacked information on whether the data field is a vector (swap is fine), or a matrix with length==1 along one dimension (swap is not fine). I added an extra input "maybevector" argument to cellmatselect. If the dimord indicates a matrix, it is set to false and the swap won't happen. Furthermore, I noticed that the low-level detection of the dimord/dimtok fails while the structure is being updated, since a selection might be made in one field but not yet in the other field. I am now passing the dimtok that is determined in the main loop to the low level selection functions. mac011> svn commit utilities/ft_selectdata.m test/test_bug2782.m Sending test/test_bug2782.m Sending utilities/ft_selectdata.m Transmitting file data .. Committed revision 10911. @Jan and Teresa, please try with the latest FieldTrip version. I tried to capture your case in the test_buf2782 script, which you might want to review. If the problem persists, please reopen.
Teresa Madsen - 2015-12-02 19:24:50 +0100
Yes, this one seems to be fixed. Thanks.
Robert Oostenveld - 2015-12-03 11:21:22 +0100
(In reply to Teresa Madsen from comment #12) thanks for the feedback!