Back to the main page.
Bug 436 - ft_rejectvisual makes inefficient (or no?) use of ft_checkdata
| Status | CLOSED INVALID |
| Reported | 2011-01-26 20:13:00 +0100 |
| Modified | 2011-05-23 10:36:50 +0200 |
| Product: | FieldTrip |
| Component: | core |
| Version: | unspecified |
| Hardware: | PC |
| Operating System: | Mac OS |
| Importance: | P1 critical |
| Assigned to: | Eelke Spaak |
| URL: | |
| Tags: | |
| Depends on: | |
| Blocks: | |
| See also: |
Robert Oostenveld - 2011-01-26 20:13:01 +0100
It does % check if the input data is valid for this function data = ft_checkdata(data, 'datatype', 'raw', 'feedback', 'yes', 'hastrialdef', 'yes', 'hasoffset', 'yes'); ... % trl is not specified in the function call, but the data is given -> % try to locate the trial definition (trl) in the nested configuration if isfield(data, 'sampleinfo') trl = [data.sampleinfo data.offset(:)]; else % a trial definition is expected in each continuous data set trl = []; warning('could not locate the trial definition ''trl'' in the data structure'); end ... % remove the offset vector if present (only applies to datasets that have been preprocessed a long time ago) if isfield(data, 'offset') data = rmfield(data, 'offset'); end this should all be doable with ft_checkdata! Other functions that appear to be similarly affected are MacBook> grep data.offset *.m ft_databrowser.m: trlorg = [data.sampleinfo data.offset(:)]; ft_rejectartifact.m: trl = [data.sampleinfo data.offset(:)]; ft_rejectvisual.m: trl = [data.sampleinfo data.offset(:)];
Eelke Spaak - 2011-05-11 09:58:03 +0200
Fixed it according to your note. However, I seem to recall you telling me that the trl-field in data structures should actually not be there (deprecated), and that sampleinfo should be used instead. That is also probably why there was a if (isfield(data,'sampleinfo')) return; end at the top of fixtrialdef.m. I have (among other things) changed the above to read if (isfield(data,'sampleinfo')) data.trl = [data.sampleinfo data.offset(:)]; return; end so that now a call to ft_checkdata with 'hastrialdef','yes' results in the data structure having a .trl-field in addition to a .sampleinfo-field, if the latter was already there. Is this the correct way? Or maybe the functions still trying to recreate a trl-matrix on the fly should instead use the sampleinfo-field (if present, or added by ft_checkdata)? (Which would be a different bug, of course.)
Jan-Mathijs Schoffelen - 2011-05-11 11:28:43 +0200
This now causes crashes here and there, because the data structure does not necessarily contain an offset field (and will only have one after it is processed by ft_checkdata with 'hasoffset', 'yes'. I personally also find this not an elegant solution because the data.trl field is redundant (can be reconstructed from the sampleinfo and the offset (which is implicit in the time vectors).
Eelke Spaak - 2011-05-11 11:46:47 +0200
I agree, this is extremely inelegant. I will revert the changes and then mark this bug as invalid, and instead make a new one something like 'remove dependence on trl matrices throughout various functions, instead use sampleinfo'. Robert and I had an email conversation about a similar issue, and will discuss in the near future (maybe at the meeting?).
Stan van Pelt - 2011-05-11 12:14:52 +0200
I think this also causes problems in ft_databrowser, since there is also a reference to (a not necessarily present) data.trl (line 176).