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

Improve testing of eigen_solve #1414

Open
kennyweiss opened this issue Sep 18, 2024 · 3 comments
Open

Improve testing of eigen_solve #1414

kennyweiss opened this issue Sep 18, 2024 · 3 comments
Labels
Core Issues related to Axom's 'core' component low priority Reviewed Testing Issues related to testing Axom

Comments

@kennyweiss
Copy link
Member

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.

@kennyweiss kennyweiss added Core Issues related to Axom's 'core' component Testing Issues related to testing Axom low priority labels Sep 18, 2024
@jcs15c
Copy link
Contributor

jcs15c commented Sep 18, 2024

As far as I can tell, taking $k < N$ doesn't pose an issue, since the method goes one-by-one through dominant eigenvalues, but there are plenty of other issues to test for.

I'll chime in with my two cents here:

  • Cent #1: The current implementation isn't built to handle complex eigenvalues, so that limits usage on any input that isn't symmetric.
  • Cent #2: My understanding of the power method is that it can't handle the case of repeated eigenvalues, so even passing it the identity matrix doesn't return the right eigenvectors. Plus, if the two dominant eigenvalues are close to each other, convergence is slow. I tested it with $$\text{diag}(1.001, 1)$$, and the default 160 iterations only gets you about 4 digits of accuracy in the eigenvalues.

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 eigen_solve is a pretty broad description relative to the capabilities of the method as is.

@kennyweiss
Copy link
Member Author

Thanks @jcs15c --
Given your caveats above, does it make sense to keep the k parameter (vs. just assuming it's N).

My take is that we should probably keep it, but improve/update the (doxygen) documentation about the limitations/expectations of eigen_solve and add tests for values other than N.

@jcs15c
Copy link
Contributor

jcs15c commented Sep 18, 2024

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 $$k=1$$, and it's cheaper to only run the power method once instead of $$N$$ times.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Issues related to Axom's 'core' component low priority Reviewed Testing Issues related to testing Axom
Projects
None yet
Development

No branches or pull requests

3 participants