Skip to content

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!

Edited by Rasmus Munk Larsen

Merge request reports

Loading