Update SVD Module to allow specifying computation options with a template parameter. Resolves #2051
Reference issue
This is related to issue #2051 (closed)
What does this implement/fix?
This refactoring is an API improvement that changes the QRPreconditioner
template parameter in JacobiSVD to a generic Options
template parameter. For consistency, it also adds a similar optional Options
parameter to BDCSVD.
This allows users to request thin unitaries for fixed-size matrices, which currently does not work. This change seems particularly beneficial when using .solve(y)
. This does not make any algorithmic changes, but it is backwards incompatible change to the existing API.
Example of Ceres having to workaround this assertion
Here is an example of how the new API gets used:
#include <Eigen/Dense>
#include <iostream>
using namespace Eigen;
using FixedSizeMatrix = Matrix<double, 4, 5>;
int main() {
FixedSizeMatrix m = FixedSizeMatrix::Random();
// Oh no! The following line has assertion because of the fixed size matrix type.
// JacobiSVD<FixedSizeMatrix> svd1(m, ComputeThinU | ComputeThinV);
JacobiSVD<FixedSizeMatrix, ColPivHouseholderQRPreconditioner | ComputeThinU | ComputeThinV> svd2(m);
std::cout << svd2.singularValues().transpose() << '\n';
return 0;
}
Additional information
For testing I mostly wanted to try all combinations of options and make sure that sizes and storage orders are set properly. It does run computation checks, but since it's not an algorithmic change I thought it would be redundant to redo everything.
I'm also a little unsure about the BDCSVD tests. It seems like the existing tests occasionally fail on the master branch if they're run a lot. Is this actually the case? They still all pass most of the time, but I think it's just the additional computation checks are making failures a little bit more common.
All feedback to improve this is greatly appreciated!