Back to the main page.
Bug 2596 - ft_sourcegrandaverage with cfg.inputfile leads to error by ft_selectdata / ft_preamble
| Status | REOPENED |
| Reported | 2014-05-29 15:26:00 +0200 |
| Modified | 2014-07-03 17:26:41 +0200 |
| Product: | FieldTrip |
| Component: | core |
| Version: | unspecified |
| Hardware: | PC |
| Operating System: | Linux |
| Importance: | P5 normal |
| Assigned to: | Eelke Spaak |
| URL: | |
| Tags: | |
| Depends on: | |
| Blocks: | |
| See also: | http://bugzilla.fcdonders.nl/show_bug.cgi?id=2054http://bugzilla.fcdonders.nl/show_bug.cgi?id=2625 |
Jens Klinzing - 2014-05-29 15:26:40 +0200
Created attachment 629 Screenshot of the error. Used script is given above. If I call ft_sourcegrandaverage and define the input via cfg.inputfile this leads to an error by ft_selectdata / ft_preamble / ft_preamble_var "cfg.inputfile should not be used in conjunction with giving input data to this function". However, I don't give input data to ft_sourcegrandaverage. I guess ft_sourcegrandaverage calls subfunctions the wrong way. In the screenshot, the result of the following script is shown: -- % First, I give the data as input arguments. The actual input data shouldn't matter I guess. s01 = load('sourceDiff_s01.mat', 'sourceDiff'); s02 = load('sourceDiff_s02.mat', 'sourceDiff'); s01 = s01.sourceDiff; s02 = s02.sourceDiff; cfg = []; cfg.parameter = 'avg.pow'; GA = ft_sourcegrandaverage(cfg, s01, s02); % This works perfectly well. Then I give the same data as cfg.inputfile. cfg.inputfile = {fullfile(pwd, 'sourceDiff_s01.mat'), fullfile(pwd, 'sourceDiff_s02.mat')}; GA = ft_sourcegrandaverage(cfg); -- Thanks and keep up the good work, Jens
Eelke Spaak - 2014-06-10 16:19:58 +0200
(In reply to jens.klinzing from comment #0) This happens because ft_selectdata specifies ft_preamble loadvar, while ft_sourcegrandaverage already calls ft_preamble loadvar before the call to ft_selectdata. My proposal is to remove loadvar from ft_selectdata, as I don't see very good use cases for that (i.e. ft_selectdata is not a true 'analysis step' in a pipeline). Alternatively, we could let all the loadvar'ing be taken care of by ft_selectdata, but that would be very messy. Robert, would you agree to remove loadvar from ft_selectdata?
Jan-Mathijs Schoffelen - 2014-06-11 13:16:11 +0200
Although loadvar'ing is probably not something that ft_selectdata should be doing, the in general best approach would be to work with a tempcfg when an ft_function is called within a ft_function. rollback provenance should then be applied so that the details of the tempcfg will be represented in the main cfg. This does not only affect ft_selectdata (being called from within an ft_function) but is a general issue.
Eelke Spaak - 2014-06-20 15:02:15 +0200
Fixed. Since we decided the programming pattern of the tmpcfg is a general one and to be adopted, I facilitated this with a simple private function. The following is now in place in ft_sourcegrandaverage in place of the previous ft_selectdata call: % ensure a consistent selection of the data over all inputs tmpcfg = maketmpcfg(cfg,... {'parameter', 'trials', 'latency', 'frequency', 'foilim'}); [varargin{:}] = ft_selectdata(tmpcfg, varargin{:}); [cfg, varargin{:}] = rollback_provenance(cfg, varargin{:}); -- bash-4.1$ svn commit ft_sourcegrandaverage.m private/maketmpcfg.m Sending ft_sourcegrandaverage.m Adding private/maketmpcfg.m Transmitting file data .. Committed revision 9645.
Jan-Mathijs Schoffelen - 2014-06-20 15:04:52 +0200
Great. I guess, this needs to be built in at other places as well...
Robert Oostenveld - 2014-06-20 15:56:28 +0200
(In reply to Eelke Spaak from comment #3) The maketmpcfg reminds me of some sub functions that I implemented in ft_selectdata and ft_checkdata, but with a more general function name. See below. I believe that they would work just fine for the tmpcfg. We could make these two functions stand-alone, and have them in fieldtrip/private and fieldtrip/utilities/private. Or I guess that they are useful enough to have them on the command line, just like getsubfield. %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% function [data] = keepfields(data, fn) fn = setdiff(fieldnames(data), fn); for i=1:numel(fn) data = rmfield(data, fn{i}); end %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% function [data] = removefields(data, fn) fn = intersect(fieldnames(data), fn); for i=1:numel(fn) data = rmfield(data, fn{i}); end
Robert Oostenveld - 2014-06-20 15:58:39 +0200
(In reply to Robert Oostenveld from comment #5) mac011> svn commit . Sending ft_checkdata.m Sending ft_selectdata.m Sending ft_selectdata_new.m Adding keepfields.m Adding removefields.m Transmitting file data ..... Committed revision 9649.
Robert Oostenveld - 2014-06-20 16:07:06 +0200
mac011> svn commit private/maketmpcfg.m utilities/ ft_sourcegrandaverage.m Sending ft_sourcegrandaverage.m Deleting private/maketmpcfg.m Sending utilities/keepfields.m Sending utilities/removefields.m Transmitting file data ... Committed revision 9651.
Robert Oostenveld - 2014-06-20 16:13:34 +0200
(In reply to Jan-Mathijs Schoffelen from comment #4) The tmpcfg construct happens a lot, and in some functions it is quite elaborate. It would indeed be a nice code cleanup to use this throughout: tmpcfg = keepfields(cfg, {...}) Hence I reopen the bug. mac011> grep -l tmpcfg'\.'.*=.*cfg'\.'\.* *.m ft_artifact_eog.m ft_artifact_jump.m ft_artifact_muscle.m ft_artifact_tms.m ft_componentanalysis.m ft_connectivityplot.m ft_databrowser.m ft_denoise_pca.m ft_dipolefitting.m ft_freqanalysis.m ft_freqstatistics.m ft_headmovement.m ft_math.m ft_megrealign.m ft_multiplotER.m ft_multiplotTFR.m ft_mvaranalysis.m ft_prepare_headmodel.m ft_prepare_leadfield.m ft_prepare_mesh.m ft_prepare_sourcemodel.m ft_rejectartifact.m ft_removetemplateartifact.m ft_singleplotER.m ft_singleplotTFR.m ft_sourceanalysis.m ft_sourceinterpolate.m ft_sourceplot.m ft_sourcestatistics.m ft_statistics_montecarlo.m ft_timelockanalysis.m ft_volumenormalise.m ft_volumerealign.m ft_volumereslice.m ft_volumesegment.m ft_volumewrite.m
Robert Oostenveld - 2014-06-20 16:14:06 +0200
(In reply to Robert Oostenveld from comment #8) and some more here mac011> grep -l tmpcfg'\.'.*=.*cfg'\.'\.* private/*.m private/rollback_provenance.m private/statistics_wrapper.m private/topoplot_common.m