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

removed valueError for nonzero eigenvalues #112

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jordandekraker
Copy link

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

ReinderVosDeWael commented Jan 29, 2024

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

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

@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
Copy link
Collaborator

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

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

Successfully merging this pull request may close these issues.

3 participants