-
Notifications
You must be signed in to change notification settings - Fork 708
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
Make unused replications warning more informative #2128
base: master
Are you sure you want to change the base?
Conversation
thanks for the suggestion. Please use ft_warning with sprintf-like argument, e.g., like this
When I run the following
I get |
I believe it should be
|
Hi Martin @martsc1 , could you make the change suggested by Robert, and test it? If it works as intended we can merge this PR! |
I get the following error when I try to use the suggested changes with a data matrix instead of a vector.
Perhaps this could be a fix: This doesn't give me an error for either a data vector or matrix, and gives the expected result. |
I see, good point. The statement is only strictly valid once all(nrepl==nrepl(1)). For now it is safe to assume this I think |
I think it is actually not safe to assume that. If the data contains NaN values for instance. I'm a bit confused now about what is being checked at this point in the function. So in the above example with the random 10x10 matrix, 7 out 10 units of observations are used, but 70 out of 70 data points are used. Which of the two should be checked here? |
the data matrix in the ft_statfun_ is always datapoint vs. observation. The datapoint dimension is a combination of spatial, spectral (frequencies), and time points. In some situations, e.g. a TFR with a frequency-varying time-window, it could be that there are some observations that contain a nan (i.e. corresponding to a time point at which no estimate could be made due to not enough data in the time window). For other frequency bins this data could be available. This would only be an issue I think, once the statistics are performed within a single subjects, across trials. If it's a 'second level' test, when the observation dimension reflects subjects, then I think it is (at this moment) safe to assume: once-a-nan, always-a-nan. we could consider to replace the nrepl(1) in the warning to mean(nrepl) and then replacing 'Using only', with Using on average' |
I still don't understand whether a replication refers to a datapoint or an observatoin? |
replication = observation, can be trial or subject, and is defined in the columns |
Thanks for the clarification! |
How about this solution:
Now the function would warn about both unused replications and unused datapoints. |
Hi @martsc1 thanks for your perseverance, I don't mind being informative, but the risk exists that people get annoyed by the amount of warnings on their screen -> ft_statfun_ will be called many times when running permutation statistics, and that will lead to many warnings, unless the user's settings are tweaked a bit to suppress repeating messages. I don't know from the top of my head what the default is. But let's not worry about that here. May I suggest a slight change of logic? You propose a nested series of if statements, which I think can be simplified to 2 non-nested if statements: if any(nrepl==0) and if any(nrepl<size(design,2) |
I'm happy with any suggestions. I think we need to modify it slightly to prevent getting replication warnings when not all datapoints are used, but all replications are. Maybe this could work:
|
Updated changes based on commit discussion.
whitespace
I think this is almost ready to be merged, were it not for the fact that for overall code consistency we might want to consider to also add similar heuristics to the other (in)depsamples statfun (F and T) |
Hi Martin, @martsc1 would you be up for making similar suggestions for the other 'statfuns', so that we can push this forward, and don't lose the work that you have spent on this up until now? |
Enhancement.
Tested once without creating errors. Imo this adds useful additional information to the warning.