cvai-roig-lab / Net2Brain

BSD 3-Clause "New" or "Revised" License
71 stars 12 forks source link

Possible bug in RDM computation?!?!?! #62

Closed sari-saba-sadiya closed 6 months ago

sari-saba-sadiya commented 6 months ago

I might be mixing something simple up, but the Pearson correlations RDM I am doing by hand are not aligning with what I am getting from our code.

Consider: x = np.array([[1,2,3],[4,4,6],[2,4,2]])

This is an n x d vector where the two first vectors are [1,2,3] and [4,4,6]. Which should have a correlation of 0.8666. I did that by hand and also checked online (https://www.statskingdom.com/correlation-calculator.html)

However running standardize from dist_util:276 I get: tensor([[-1.0690, -1.4142, -0.3922], [ 1.3363, 0.7071, 1.3728], [-0.2673, 0.7071, -0.9806]])

Which is obviously incorrect, replacing the default dim 0 to 1 however I get: tensor([[-1.2247, 0.0000, 1.2247], [-0.7071, -0.7071, 1.4142], [-0.7071, 1.4142, -0.7071]])

Which is right, looking at dist.py line 166 however we can see it is called with dim 0 and then is used with torch.corrcoef.

Ofcourse, we don't even need the standardize here since corrcoeff fixes that anyway. But I am worried that other places in the code call the method with distances that will not be standarizing automatically.

Anyway, since this really effected my results I am worried that a few projects might have been effected. Could someone validate this ASAP? sorry for the trouble

sari-saba-sadiya commented 6 months ago

Ok, I see that this was added recently as an implementation of what Kriegeskorte had. Dom already fixed this in dev, so I will close the issue.

Btw, this is not the default in rsatoolbox, and the 2008 paper seems a bit ambiguous, lets talk about this tomorrow