Skip to content
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

limo_display_results() showing insignificant clusters too? #95

Open
NirOfir opened this issue Jan 3, 2022 · 5 comments
Open

limo_display_results() showing insignificant clusters too? #95

NirOfir opened this issue Jan 3, 2022 · 5 comments

Comments

@NirOfir
Copy link
Contributor

NirOfir commented Jan 3, 2022

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
Copy link
Contributor

CPernet commented Jan 10, 2022

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

@NirOfir
Copy link
Contributor Author

NirOfir commented Jan 12, 2022

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
Copy link
Contributor

CPernet commented Jan 12, 2022

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
Copy link
Contributor

CPernet commented Jan 12, 2022

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

@NirOfir
Copy link
Contributor Author

NirOfir commented Jan 13, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants