Improve sparseJacobianEigen()
#682
Conversation
Also, the unit test changed due to a 0 entry that was previously wrongly included in the b-column of the sparse representation.
About 5% speed improvement.
Templating still used in cpp file for generic-ness, but not exposed anymore std::tuple has the same performance as Eigen::Triplet, but boost::tuple is slower. Sparse matrix indices are int instead of size_t for efficiency (e.g. A(i, j) = s -> i/j are int's instead of size_t's)
There was a problem hiding this comment.
Gerry, I appreciated the enthusiasm, but it seems you created a (confusing) zoo of options for some hypothetical user in the future. Let's not: we need an opinionated API that provides just what is needed. Try to remember why this PR exists, and satisfy that use case. Leave everything else untouched. I think we can change to your "fast type" without changing the API (substantially).
gtsam/linear/GaussianFactorGraph.h
Outdated
| SparseMatrixBoostTriplets; | ||
| /// Sparse matrix representation as vector of slightly more efficient | ||
| /// tuples. | ||
| typedef std::vector<std::tuple<int, int, double>> SparseMatrixGtsamTriplets; |
There was a problem hiding this comment.
What's the reasoning here? I'd prefer to keep only one of these.
There was a problem hiding this comment.
Originally the code used SparseMatrixBoostTriplets. I added SparseMatrixGtsamTriplets because it's about 14% faster and 33% less memory intensive (16 bytes instead of 24 bytes per nnz entry) in my tests. Reason: 1) size_t is 8 bytes, and even if we use int instead, boost::tuple aligns to 8 bytes. We don't have to typedef these?
Old version kept for backwards compatibility.
There was a problem hiding this comment.
I removed the typedefs but both are still used (the std::tuple version is used in the "fast" version).
|
Nice! |
#677 contained copy/pasted code from
GaussianFactorGraph::sparseJacobianinsparseJacobianEigen. This PR makes the more generic,GaussianFactorGraph::sparseJacobianInPlace<T>(T& entries, ...)and uses it instead insparseJacobianEigen.It also adds an optional Ordering argument to better match the other jacobian functions, and optional nrows/ncols parameters since these are necessary to keep track of for sparse matrices.
Finally, efficiency is slightly improved by inserting directly as described here, benchmarked by running
sparseJacobianEigenon a linearizedBALfinal-394-100368-pre:Tasvector<Eigen::Triple>and usingsetFromTriplets(original)Tasvector<boost::tuple>and usingSparseEigen::insertTasvector<Eigen::Triple>and usingSparseEigen::insert(new)Notes about the diff:
sparseJacobiantosparseJacobianInPlacewith the following modifications:entriesis no longer constructedcol/rowvariables were renamed tonrowsandncolsto match parameter namesbvector are only added if they are non-zerotestGaussianFactorGraphone entry A(5, 7) = 0 was removed since it has value 0 (see previous bullet point)