MICA-MNI / BrainSpace

BrainSpace is an open-access toolbox that allows for the identification and analysis of gradients from neuroimaging and connectomics datasets | available in both Python and Matlab |
http://brainspace.readthedocs.io
BSD 3-Clause "New" or "Revised" License
180 stars 72 forks source link

removed valueError for nonzero eigenvalues #112

Open jordandekraker opened 7 months ago

jordandekraker commented 7 months ago

I was rtying to run this for hippocampal surfaces with no masked data (i.e. no medial wall) and received an error on this line. I believe it is an unneeded line.

ReinderVosDeWael commented 7 months ago

I'd be more curious as to why this is happening in the first place. The MSM manuscript is not ambiguous:

A set of n observations with a full-rank spatial weights matrix W of size n × n will result in n – 1 orthogonal and uncorrelated (Griffith 2000) eigenvectors Vk associated with eigenvalues λk, while a single eigenvector with zero eigenvalue is dropped.

Unfortunately, this seems like the kind of deep dive that's beyond what I have time for these days. That being said, I'd see if you have duplicate data in your input. My first instinct is that that's what's going on.

jordandekraker commented 7 months ago

Yes there are likely duplicate values, but this is valid in my case. All my vertices were generated on one regular meshgrid (and then some vertices were removed, leading to appropriate face sizes in hippocampal meshes). Thus, some vertices will have identical "n_ring=1" distances (or in other words, many face sizes are identical)

It looks like the next few lines are meant to handle cases with many 0 Eigenvalues anyway, so I really think this line can just be dropped.

Not sure why the CI has broken

ReinderVosDeWael commented 7 months ago

@OualidBenkarim What are your thoughts; I think we could consider just changing it to a warning. This shouldn't happen, and is likely indicative of another issue, but if the end-user believes their use-case is an exception we can allow it.

FYI these are my fairly uninformed thoughts coalesced in the ten minutes I could find to look at this, so don't put too much stock in my opinion on this :P

OualidBenkarim commented 7 months ago

There's the tol argument to control that. Maybe increasing the tolerance might fix the issue