-
Notifications
You must be signed in to change notification settings - Fork 732
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Docstring fix: unsupported option #2268
base: master
Are you sure you want to change the base?
Conversation
hmm, it should be used to allow computational reproducibility of the exact same random sequence. The same option is also used in |
Ah, it is using this
which then causes these to do the actual work
|
Although it is technically set at the level of the ft_XXXstatistics wrapper, the 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... |
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. |
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 |
Clarification: it has been shown not work, when calling ft_statistics_montecarlo directly, i.e. bypassing the high(est) level ft function |
but if you call |
hmm, this also fails
on the second assert. That suggests that the randomseed is not set properly. |
I came across this when calling Since EDIT: This is a cross-post with @robertoostenveld's latest post on failures, which I only saw after posting this. |
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
so to go through the wrapper. |
Yes absolutely, and I am well aware that my approach is not the standard approach. |
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. |
…, also added to help.
ft_montecarlostatistic
listscfg.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?