-
Notifications
You must be signed in to change notification settings - Fork 27
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
Improve testing of eigen_solve #1414
Comments
As far as I can tell, taking I'll chime in with my two cents here:
All that said, these issues end up pretty tolerable in the context of the OBB usage: the covariance matrix passed as input is always SPD so you get an orthonormal basis out of it either way, and the relevant constructor doesn't even use the eigenvalues. You don't necessarily get the best OBB (since the eigenvectors might not be correct), but it's at least a valid one that contains all the points, and is still usually better than the AABB. I think the utility of this method would go a long way if we just restricted it to symmetric input with some generous warning about its usage. Generally, I think |
Thanks @jcs15c -- My take is that we should probably keep it, but improve/update the (doxygen) documentation about the limitations/expectations of |
I think it makes sense. There are probably situations where you only need a single eigenvalue/vector pair, maybe if you're trying to compute some sort of spectral norm? In that case you'd only need |
When reviewing #1412, which fixes a bug in the
eigen_solve
routine, I noticed that we do not have any tests where the number of desired eigenvalues/vectors,k
, is less than the dimension,N
of the square matrix,A
.We should test that this works and produces the correct results.
The text was updated successfully, but these errors were encountered: