GTSAM 4 crashes with zero dimension factors

We use zero dimension factors with GTSAM 3.2.1 (can sound stupid but is actually very helpful for genericity purposes), and it works fine. When upgrading to GTSAM 4.0.0-alpha2 (required to upgrade to Eigen 3.3.x), we found that GTSAM 4 was crashing with these factors.

We found two changes required to make it work. One related to the CachedModel size:

--- gtsam/nonlinear/internal/LevenbergMarquardtState.h.orig     2017-08-05 22:20:16.000000000 +0200
+++ gtsam/nonlinear/internal/LevenbergMarquardtState.h  2018-09-04 10:42:15.506961159 +0200
@@ -113,9 +113,9 @@
   // Small cache of A|b|model indexed by dimension. Can save many mallocs.
   mutable std::vector<CachedModel> noiseModelCache;
   CachedModel* getCachedModel(size_t dim) const {
-    if (dim > noiseModelCache.size())
-      noiseModelCache.resize(dim);
-    CachedModel* item = &noiseModelCache[dim - 1];
+    if (dim >= noiseModelCache.size())
+      noiseModelCache.resize(dim+1);
+    CachedModel* item = &noiseModelCache[dim];
     if (!item->model)
       *item = CachedModel(dim, 1.0 / std::sqrt(lambda));
     return item;

and another one to just accept them:

--- gtsam/linear/Scatter.cpp.orig       2017-08-05 22:20:16.000000000 +0200
+++ gtsam/linear/Scatter.cpp    2018-09-04 11:57:18.284827955 +0200
@@ -72,7 +72,7 @@
   if (first != end()) std::sort(first, end());

   // Filter out keys with zero dimensions (if ordering had more keys)
-  erase(std::remove_if(begin(), end(), SlotEntry::Zero), end());
 }

Thus:

Does it make sense for you to accept these zero dimension factors ?
Do you see possible bad side effects of this fix ? (GTSAM unit tests pass)

If it seems correct to you, I will happily provide a PR including these changes.

Comments (5)