search

LIMO-EEG-Toolbox / limo_tools

Hierarchical Linear Modelling for MEEG data
https://limo-eeg-toolbox.github.io/limo_meeg/
Other
58 stars 27 forks source link

limo_display_results() showing insignificant clusters too? #95

Open NirOfir opened 2 years ago

NirOfir commented 2 years ago

That's a bit of a weird bug that I don't understand completely yet. I noticed it happen only once so far.

Whether or not a cluster is significant is determined by limo_cluster_test() lines 50-60, by comparing to a value (maxclustersum_th) which is larger than 95% of the bootstrap distribution. The p values themselves are calculated later in lines 70-83 by calculating 1-(number of times the observed statistic of the cluster was larger than the bootstrap values). For some reason, some of the clusters that were found to be significant by lines 50-60 get p values that are larger than 0.05! I'm using the latest git version, and there are no NaNs in the bootstrap distribution.

I don't really understand why it happens, but I wanted to let you know while I'm trying to figure it out, in case you have a clear answer seems.

Thanks!

CPernet commented 2 years ago

is that for our new positive vs negative T distrib? from your branch

NirOfir commented 2 years ago

Yep, but I didn't really expect it to matter, since both computations (finding the threshold and calculating the p values) are based on the same distribution, and both are sound as far as I can tell...

CPernet commented 2 years ago

but it does --> maxclustersum_th > 95% bootstrap T^2 = 1-(number of times the observed statistic of the cluster was larger than the bootstrap values)

now you compute maxclustersum_th for T+ and maxclustersum_th for T- separately, what about the bootstrap? it should be only based on bootT+ or bootT- to match the observed data

CPernet commented 2 years ago

PS: this is why I haven't merged yet -- i need to set up simulations and do testing to avoid such surprises

NirOfir commented 2 years ago

My code does it separately for T+ and T- (following the specific implementation in fieldtrip), but you're right that the merge should wait until verified. For now, I don't want to wait too much of your time, so I'll let you know if I manage to replicate this bug reliably.