Fix SymmetricEigen Routine

[CattleProdigy]

I've got some proposed fixes to address the issue referenced below.

The bug itself is in the the subdim=2 special case. As best as I can tell, in that block we try to simply replace the unreduced block by its diagonalization, with the eigenvectors (called basis in the code) serving a similar role as a Givens rotation does in the Implicit-Shift QR part of the algorithm. ~~The eigenvector / eigenvalue correspondence is mismatched. Unfortunately, upon fixing that I find the proptests break regularly. The results are correct but not to within the specified threshold.~~ I should also note that the proptests are also failing on dev though you have to run it a few times to see a failure. There was nothing in theory wrong with the existing calculation, but it has numerical issues. In the SVD version of this algorithm, a special 2x2 SVD implementation is used which avoid catastrophic cancellation. I adjusted the subdim=2 case to similarly select the eigenvector which bests avoid cancellation.

I'm not sure ~~if there's a better way to salvage the subdim=2 case or~~ if there's any advantage to doing so. In any case, I ultimately just removed it and let the Implicit-Shift QR part handle everything. That passes the test case from the user who reported the issue and proptest passes (100s of times without failing). FWIW the implementation in Eigen doesn't seem to have this special case.

For feedback:

• Do we want to keep the user-submitted test case?
• Do we want to try to keep the part I removed?

Addresses https://github.com/dimforge/nalgebra/issues/1109

[Andlon]

I haven't yet had time to review this, but I also don't know the specific motivation for the subdim = 2 special case. @sebcrozet, would you be able to comment on the motivation for this? Is it e.g. significantly faster in some cases important to you?

[Andlon]

@CattleProdigy: as I said on Discord, thank you so much for looking into this! I hope to be able to review soon, but in the meantime I'll try to answer one more of your questions. I think we definitely want to keep all user-submitted test cases in the test suite, with a clear reference to the issue in question, e.g. symmetric_eigen_regression_issue_1109() or similar (in a way that aligns with the current naming conventions for the other SymmetricEigen tests, I haven't looked it up).

[CattleProdigy]

@CattleProdigy: as I said on Discord, thank you so much for looking into this! I hope to be able to review soon, but in the meantime I'll try to answer one more of your questions. I think we definitely want to keep all user-submitted test cases in the test suite, with a clear reference to the issue in question, e.g. symmetric_eigen_regression_issue_1109() or similar (in a way that aligns with the current naming conventions for the other SymmetricEigen tests, I haven't looked it up).

I've updated the name of the test and the docstrings to match the other issue/regression tests

[CattleProdigy]

@Andlon @sebcrozet

I've decided to adopt the solution that disrupts the code the least: maintaining the subdim=2 case. I synced my branch with dev. It passes the regression test and the linalg::eigen proptests. It should be ready to review and merge.

[Andlon]

Thanks for working on this @CattleProdigy, this is really great. I am not at all familiar with the code here, so given the subtlety of eigenvalue routines, I don't feel quite comfortable merging it right away, although of course the passing tests give great confidence to the solution here. Still, @sebcrozet, I think you were the original author of these routines, would you be able to take a look?