LLNL / axom

CS infrastructure components for HPC applications
BSD 3-Clause "New" or "Revised" License
157 stars 27 forks source link

Improve testing of eigen_solve #1414

Open kennyweiss opened 1 month ago

kennyweiss commented 1 month ago

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.

jcs15c commented 1 month ago

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:

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 commented 1 month ago

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 commented 1 month ago

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.