Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 (calledbasis
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 ondev
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 theif 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.subdim=2
case orFor feedback:
Addresses #1109