commit 12eb389b4ec4113a2260c1a192a1d3f8d1b6a2d3
author Sameer Agarwal <sameeragarwal@google.com> Wed Aug 27 22:18:33 2014 -0700
committer Keir Mierle <mierle@gmail.com> Thu Aug 28 16:34:22 2014 +0000
tree f835233316a60053928a388247c04d935fd0f17a
parent 6c25185bb1643d8d0f3d8e1a7b82a058156aa869

Fix Eigen Row/ColMajor bug in NumericDiffCostFunction.

If the parameter block size is 1, asking Eigen to create
a row-major matrix triggers a compile time error. Previously
we were handling the case where the number of rows in the
jacobian block was known statically, but the problem is present
when the nummber of rows is dynamic.

This CL fixes this problem.

Thanks to Dominik Reitzle for reporting this.

Change-Id: I99c3eec3558e66ebf4efa51c4dee8ce292ffe0c1

include/ceres/internal/numeric_diff.h

diff --git a/include/ceres/internal/numeric_diff.h b/include/ceres/internal/numeric_diff.h
index 5048348..3b264b4 100644
--- a/include/ceres/internal/numeric_diff.h
+++ b/include/ceres/internal/numeric_diff.h
@@ -103,14 +103,17 @@
 
     typedef Matrix<double, kNumResiduals, 1> ResidualVector;
     typedef Matrix<double, kParameterBlockSize, 1> ParameterVector;
+
+    // The convoluted reasoning for choosing the Row/Column major
+    // ordering of the matrix is an artifact of the restrictions in
+    // Eigen that prevent it from creating RowMajor matrices with a
+    // single column. In these cases, we ask for a ColMajor matrix.
     typedef Matrix<double,
                    kNumResiduals,
                    kParameterBlockSize,
-                   (kParameterBlockSize == 1 &&
-                    kNumResiduals > 1) ? ColMajor : RowMajor>
+                   (kParameterBlockSize == 1) ? ColMajor : RowMajor>
         JacobianMatrix;
 
-
     Map<JacobianMatrix> parameter_jacobian(jacobian,
                                            NUM_RESIDUALS,
                                            kParameterBlockSize);

internal/ceres/numeric_diff_cost_function_test.cc

diff --git a/internal/ceres/numeric_diff_cost_function_test.cc b/internal/ceres/numeric_diff_cost_function_test.cc
index 422c712..b3d8817 100644
--- a/internal/ceres/numeric_diff_cost_function_test.cc
+++ b/internal/ceres/numeric_diff_cost_function_test.cc
@@ -182,6 +182,30 @@
   cost_function.reset(
       new NumericDiffCostFunction<SizeTestingCostFunction<2,2>,  CENTRAL, 2, 2>(
           new SizeTestingCostFunction<2,2>, ceres::TAKE_OWNERSHIP));
+
+  cost_function.reset(
+      new NumericDiffCostFunction<EasyFunctor, CENTRAL, ceres::DYNAMIC, 1, 1>(
+          new EasyFunctor, TAKE_OWNERSHIP, 1));
+
+  cost_function.reset(
+      new NumericDiffCostFunction<EasyFunctor, CENTRAL, ceres::DYNAMIC, 1, 1>(
+          new EasyFunctor, TAKE_OWNERSHIP, 2));
+
+  cost_function.reset(
+      new NumericDiffCostFunction<EasyFunctor, CENTRAL, ceres::DYNAMIC, 1, 2>(
+          new EasyFunctor, TAKE_OWNERSHIP, 1));
+
+  cost_function.reset(
+      new NumericDiffCostFunction<EasyFunctor, CENTRAL, ceres::DYNAMIC, 1, 2>(
+          new EasyFunctor, TAKE_OWNERSHIP, 2));
+
+  cost_function.reset(
+      new NumericDiffCostFunction<EasyFunctor, CENTRAL, ceres::DYNAMIC, 2, 1>(
+          new EasyFunctor, TAKE_OWNERSHIP, 1));
+
+  cost_function.reset(
+      new NumericDiffCostFunction<EasyFunctor, CENTRAL, ceres::DYNAMIC, 2, 1>(
+          new EasyFunctor, TAKE_OWNERSHIP, 2));
 }
 
 TEST(NumericDiffCostFunction, EasyCaseFunctorCentralDifferencesAndDynamicNumResiduals) {