additional sparseJacobian functions

[gchenfc]

The primary purpose of this PR is to make a sparseJacobian function in GaussianFactorGraph that returns an Eigen::SparseMatrix. This is useful for interfacing with third-party linear solvers (#111) such as Eigen's sparse factorizer, and CUDA-accelerated linear solvers like SuiteSparse and cuSPARSE.

Currently, there are already two functions:

std::vector<boost::tuple<size_t, size_t, double> > sparseJacobian() const;
Matrix sparseJacobian_() const;

But I would argue that this naming scheme isn't great. Instead, I propose to template the function on the type of output we want. So,

template <typename Entries>
Entries sparseJacobian() const;

typedef std::vector<boost::tuple<size_t, size_t, double>>
    SparseMatrixBoostTriplets;
typedef std::vector<Eigen::Triplet<double>> SparseMatrixEigenTriplets;
typedef Eigen::SparseMatrix<double, Eigen::ColMajor> SparseMatrixEigen;

Then it would be used:

auto jacobian = fg.sparseJacobian<SparseMatrixEigen>(ordering);
auto jacobian2 = fg.sparseJacobian<SparseMatrixEigenTriplets>(ordering);
...

That is the gist.

---

A couple (controversial) implementation details:

• sparseJacobian is actually returning a 3-tuple since I found that the dimensions of the sparse matrix are often useful. An alternative would be passing by ("optional") reference
• Template declarations/specializations are in the cpp file. Works fine and I argue it's reasonable here because there's really only a finite number of valid template arguments. Basically this is "overloading the return type".
• Overall it's a little bit uglier than I would have hoped.

[ProfFan]

Just my first impression tho, why tag dispatch instead of multiple functions with different names?

[gchenfc]

Just my first impression tho, why tag dispatch instead of multiple functions with different names?

I felt it would be awkward to have names like

sparseJacobian()              // vector of boost::tuple
sparseJacobian_()             // matrix
sparseJacobianEigen_()        // Eigen::SparseMatrix
sparseJacobianEigenTriples_() // vector of Eigen::Triple's

And figured

sparseJacobian <vector<boost::tuple>> ()
sparseJacobian <Matrix> ()
sparseJacobian <Eigen::SparseMatrix> ()
sparseJacobian <vector<Eigen::Triple>> ()

would be cleaner. But not sure.

Ok, Frank - yes I did find compilation times to get way slower on my computer so I will change this. Thx for the tip!

[dellaert]

We also need to make sure to be backward compatible.

[dellaert]

Just my first impression tho, why tag dispatch instead of multiple functions with different names?

I felt it would be awkward to have names like

sparseJacobian()              // vector of boost::tuple
sparseJacobian_()             // matrix
sparseJacobianEigen_()        // Eigen::SparseMatrix
sparseJacobianEigenTriples_() // vector of Eigen::Triple's

And figured

sparseJacobian <vector<boost::tuple>> ()
sparseJacobian <Matrix> ()
sparseJacobian <Eigen::SparseMatrix> ()
sparseJacobian <vector<Eigen::Triple>> ()

would be cleaner. But not sure.

I'm with Fan on this. Will also cause problems with wrapper. (which you have to be aware of)

[dellaert]

@gchenfc ping on this. Please re-request review once comments have been addressed.

[gchenfc]

I changed the names back, so now the relevant functions are:

gfg.sparseJacobian()              // vector of boost::tuple
gfg.sparseJacobian_()             // matrix
SparseMatrix::JacobianEigen(gfg) // Eigen::SparseMatrix

Now SparseMatrix.h is the only file that includes <Eigen/Sparse>. I don't think it makes much sense to move that include to SparseMatrix.cpp since any time you want a function from SparseMatrix, you should also need Eigen::SparseMatrix.

I also added additional useful arguments for ordering and dimensions.

[dellaert]

I appreciate you trying to "do the right thing", but it just comes across as heavy handed here, with the class and static methods. This is supposed to be a simple PR, not a heavy handed one :-) I think you should just move the static functions to gfg.cpp, unless there is a good reason.

Light-handed approach philosophy: "Don't build for the future, but the now". The now only needs gfg unit tests and functionality, so this is what this PR should be.

[dellaert]

Move additions in this file to SparseMatrix.h...

[dellaert]

Why a class?

[dellaert]

base cannot know about linear :-/ the directories in GTSAm form a directed graph.
I think all these are utilities for GaussianFactorGraph functions, right? Then why not make them static functions inside GaussianFactorGraph.cpp? Only expose the type (which you exposed in Matrix.h) in the GaussianFactorGraph.h header.

[gchenfc]

I appreciate you trying to "do the right thing", but it just comes across as heavy handed here, with the class and static methods. This is supposed to be a simple PR, not a heavy handed one :-) I think you should just move the static functions to gfg.cpp, unless there is a good reason.

Light-handed approach philosophy: "Don't build for the future, but the now". The now only needs gfg unit tests and functionality, so this is what this PR should be.

Yes sounds great - I originally wanted to put them in GaussianFactorGraph.cpp but you made an earlier comment that you wanted to avoid that if possible so I tried without, which was very ugly. I knew the extra file was ugly too and I really didn't like it.

[gchenfc]

@dellaert To clarify what I assume is your original reasoning for not wanting to place these static functions in GaussianFactorGraph directly, either GaussianFactorGraph.h will have to #include <Eigen/Sparse> or we will have to have a message to warn users that use the function to include it themselves

[dellaert]

@dellaert To clarify what I assume is your original reasoning for not wanting to place these static functions in GaussianFactorGraph directly, either GaussianFactorGraph.h will have to #include <Eigen/Sparse> or we will have to have a message to warn users that use the function to include it themselves

Right. You can add a new file in linear subdirectory that is focused on the Eigen functionality, so we don't include Eigen/Sparse in gfg.h. Given the other utilities, I could see it even be a simple header only file.

[dellaert]

Let’s fix and land?

[gchenfc]

Switched to #677 to reduce commit- and diff- pollution