Docstring fix: unsupported option

[britta-wstnr]

ft_montecarlostatistic lists cfg.randomseed as an option in the docstring, which has no effect and is not used in the function.

I deleted it - should there be a warning thrown if someone sets it?

[robertoostenveld]

hmm, it should be used to allow computational reproducibility of the exact same random sequence. The same option is also used in ft_componentanalysis. Might it be that it is used in a hidden fashion in a preamble?

[robertoostenveld]

Ah, it is using this

mac036> grep 'ft_preamble randomseed' *.m
ft_componentanalysis.m:ft_preamble randomseed
ft_connectivitysimulation.m:ft_preamble randomseed
ft_dipolesimulation.m:ft_preamble randomseed
ft_freqsimulation.m:ft_preamble randomseed
ft_freqstatistics.m:ft_preamble randomseed
ft_sourcestatistics.m:ft_preamble randomseed
ft_timelockstatistics.m:ft_preamble randomseed

which then causes these to do the actual work

utilities/private/ft_preamble_randomseed.m 
utilities/private/randomseed.m 

[robertoostenveld]

Although it is technically set at the level of the ft_XXXstatistics wrapper, the cfg.randomseed option is only used in the ft_statistics_montecarlo and hence it is documented at that level.

If you agree, I suggest to close this PR. Unless of course you have experimentally found for it not to work, because then there might be an actual bug...

[schoffelen]

Welll, it has been experimentally confirmed that it does not work: grepping ft_statistics_montecarlo for randomseed only gives a hit in the help section.

ft_timelock/freq/sourcestatistics do work with cfg.randomseed and the respective ambles, as far as I can see.

Therefore, I thought that the help in ft_statistics_montecarlo was just a leftover from the old days.

[robertoostenveld]

you have to grep three layers deep to find it. I don't think it is a left-over. We have some test scripts that use the cfg.randomseed option for ICA, perhaps we should also make a test script for the other uses (in simulation and montecarlo stats).

[schoffelen]

Clarification: it has been shown not work, when calling ft_statistics_montecarlo directly, i.e. bypassing the high(est) level ft function

[robertoostenveld]

but if you call ft_statistics_montecarlo directly, you get a warning that that is not what you should be doing, right?

[robertoostenveld]

hmm, this also fails

cfg = [];
cfg.s1.numcycli = 1;
cfg.s1.ampl     = 1.0;
cfg.s2.numcycli = 2;
cfg.s2.ampl     = 0.7;
cfg.s3.numcycli = 4;
cfg.s3.ampl     = 0.2;
cfg.noise.ampl  = 0.1;
data1 = ft_timelocksimulation(cfg);

cfg = [];
cfg.s1.numcycli = 1;
cfg.s1.ampl     = 1.0;
cfg.s2.numcycli = 2;
cfg.s2.ampl     = 0.7;
cfg.s3.numcycli = 4;
cfg.s3.ampl     = 0.2;
cfg.noise.ampl  = 0.1;
data2 = ft_timelocksimulation(cfg);

% they should have different noise
assert(~isequal(data1.trial, data2.trial));

%%

cfg = [];
cfg.s1.numcycli = 1;
cfg.s1.ampl     = 1.0;
cfg.s2.numcycli = 2;
cfg.s2.ampl     = 0.7;
cfg.s3.numcycli = 4;
cfg.s3.ampl     = 0.2;
cfg.noise.ampl  = 0.1;
cfg.randomseed  = 123;
data1 = ft_timelocksimulation(cfg);

cfg = [];
cfg.s1.numcycli = 1;
cfg.s1.ampl     = 1.0;
cfg.s2.numcycli = 2;
cfg.s2.ampl     = 0.7;
cfg.s3.numcycli = 4;
cfg.s3.ampl     = 0.2;
cfg.noise.ampl  = 0.1;
cfg.randomseed  = 123;
data2 = ft_timelocksimulation(cfg);

% they should have the same noise
assert(isequal(data1.trial, data2.trial));

on the second assert. That suggests that the randomseed is not set properly.

[britta-wstnr]

I came across this when calling ft_statistics_montecarlo directly (which I do because higher level functions do not allow me to use the data I am using (doing stats on connectivity).
Here, it fails to give me reproducible permutations.

Since cfg.randomseed is not used in ft_statistics_montecarlo at all (but apparently at a higher level function), I would still suggest to delete it here.

EDIT: This is a cross-post with @robertoostenveld's latest post on failures, which I only saw after posting this.

[britta-wstnr]

but if you call ft_statistics_montecarlo directly, you get a warning that that is not what you should be doing, right?

I don't think I get a warning from that.

[robertoostenveld]

but if you call ft_statistics_montecarlo directly, you get a warning that that is not what you should be doing, right?

I don't think I get a warning from that.

Perhaps those warnings were only implemented for the old (now removed) implementation of the different freqanalysis methods, which in the past were called by ft_freqanalysis. I do think that it is good coding style to give warnings when people inadvertently use the code in creative (i.e., unintended or undocumented) ways. People that know what they are doing are of course welcome to proceed anyway. The ft_statistics_montecarlo function documents that

% FT_STATISTICS_MONTECARLO performs a nonparametric statistical test by calculating
% Monte-Carlo estimates of the significance probabilities and/or critical values from
% the permutation distribution. This function should not be called directly, instead
% you should call the function that is associated with the type of data on which you
% want to perform the test.
%
% Use as
%   stat = ft_timelockstatistics(cfg, data1, data2, data3, ...)
%   stat = ft_freqstatistics    (cfg, data1, data2, data3, ...)
%   stat = ft_sourcestatistics  (cfg, data1, data2, data3, ...)

so to go through the wrapper.

[britta-wstnr]

I do think that it is good coding style to give warnings when people inadvertently use the code in creative (i.e., unintended or undocumented) ways. People that know what they are doing are of course welcome to proceed anyway. The ft_statistics_montecarlo function documents that

Yes absolutely, and I am well aware that my approach is not the standard approach.
However, I still thought that documenting non-effective options in the docstring of a sub-level function might be confusing and hinder maintenance.

[robertoostenveld]

we more often use the docstrings of publicly visible sub-functions like this. Also for ft_channelselection (called by ft_selectdata, which is called by many other functions), ft_datatype_xxx (called by ft_checkdata, again called by many other functions), etc.

EDIT: and in general we use the principle consistency is more important than perfection, although this also discusses some counterpoints.