Fix a bug in DetectStructure

The logic for determing static/dynamic f-block size in
DetectStructure was broken in a corner case, where the very first
row block which was used to initialize the f_block_size contained
more than one f blocks of varying sizes. The way the if block
was structured, no iteration was performed on the remaining
f-blocks and the loop failed to detect that the f-block size
was actually changing.

If in the remaining row blocks, there were no row blocks
with varying f-block sizes, the function will erroneously
return a static f-block size.

Thanks to Johannes Schonberger for providing a reproduction for this
rather tricky corner case.

Change-Id: Ib442a041d8b7efd29f9653be6a11a69d0eccd1ec

internal/ceres/CMakeLists.txt

@@ -273,6 +273,7 @@ if (BUILD_TESTING AND GFLAGS)
   ceres_test(cost_function_to_functor)
   ceres_test(covariance)
   ceres_test(cubic_interpolation)
+  ceres_test(detect_structure)
   ceres_test(dense_sparse_matrix)
   ceres_test(dynamic_autodiff_cost_function)
   ceres_test(dynamic_compressed_row_sparse_matrix)

internal/ceres/detect_structure.cc

@@ -55,6 +55,7 @@ void DetectStructure(const CompressedRowBlockStructure& bs,
       break;
     }
 
+    // Detect fixed or dynamic row block size.
     if (*row_block_size == 0) {
       *row_block_size = row.block.size;
     } else if (*row_block_size != Eigen::Dynamic &&
@@ -65,6 +66,7 @@ void DetectStructure(const CompressedRowBlockStructure& bs,
       *row_block_size = Eigen::Dynamic;
     }
 
+    // Detect fixed or dynamic e-block size.
     const int e_block_id = row.cells.front().block_id;
     if (*e_block_size == 0) {
       *e_block_size = bs.cols[e_block_id].size;
@@ -76,19 +78,24 @@ void DetectStructure(const CompressedRowBlockStructure& bs,
       *e_block_size = Eigen::Dynamic;
     }
 
-    if (*f_block_size == 0) {
-      if (row.cells.size() > 1) {
+    // Detect fixed or dynamic f-block size. We are only interested in
+    // rows with e-blocks, and the e-block is always the first block,
+    // so only rows of size greater than 1 are of interest.
+    if (row.cells.size() > 1) {
+      if (*f_block_size == 0) {
         const int f_block_id = row.cells[1].block_id;
         *f_block_size = bs.cols[f_block_id].size;
       }
-    } else if (*f_block_size != Eigen::Dynamic) {
-      for (int c = 1; c < row.cells.size(); ++c) {
-        if (*f_block_size != bs.cols[row.cells[c].block_id].size) {
+
+      for (int c = 1;
+           (c < row.cells.size()) && (*f_block_size != Eigen::Dynamic);
+           ++c) {
+        const int f_block_id = row.cells[c].block_id;
+        if (*f_block_size != bs.cols[f_block_id].size) {
           VLOG(2) << "Dynamic f block size because the block size "
                   << "changed from " << *f_block_size << " to "
-                  << bs.cols[row.cells[c].block_id].size;
+                  << bs.cols[f_block_id].size;
           *f_block_size = Eigen::Dynamic;
-          break;
         }
       }
     }

internal/ceres/detect_structure_test.cc

@@ -0,0 +1,294 @@
+// Ceres Solver - A fast non-linear least squares minimizer
+// Copyright 2015 Google Inc. All rights reserved.
+// http://ceres-solver.org/
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are met:
+//
+// * Redistributions of source code must retain the above copyright notice,
+//   this list of conditions and the following disclaimer.
+// * Redistributions in binary form must reproduce the above copyright notice,
+//   this list of conditions and the following disclaimer in the documentation
+//   and/or other materials provided with the distribution.
+// * Neither the name of Google Inc. nor the names of its contributors may be
+//   used to endorse or promote products derived from this software without
+//   specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+// AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+// IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+// ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
+// LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+// CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+// SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+// INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+// CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+// ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+// POSSIBILITY OF SUCH DAMAGE.
+//
+// Author: sameeragarwal@google.com (Sameer Agarwal)
+
+#include "Eigen/Core"
+#include "glog/logging.h"
+#include "gtest/gtest.h"
+#include "ceres/block_structure.h"
+#include "ceres/detect_structure.h"
+
+namespace ceres {
+namespace internal {
+
+TEST(DetectStructure, EverythingStatic) {
+  const int expected_row_block_size = 2;
+  const int expected_e_block_size = 3;
+  const int expected_f_block_size = 4;
+
+  CompressedRowBlockStructure bs;
+
+  bs.cols.push_back(Block());
+  bs.cols.back().size = 3;
+  bs.cols.back().position = 0;
+
+  bs.cols.push_back(Block());
+  bs.cols.back().size = 4;
+  bs.cols.back().position = 3;
+
+  bs.cols.push_back(Block());
+  bs.cols.back().size = 4;
+  bs.cols.back().position = 7;
+
+  {
+    bs.rows.push_back(CompressedRow());
+    CompressedRow& row = bs.rows.back();
+    row.block.size = 2;
+    row.block.position = 0;
+    row.cells.push_back(Cell(0, 0));
+    row.cells.push_back(Cell(1, 0));
+  }
+
+  {
+    bs.rows.push_back(CompressedRow());
+    CompressedRow& row = bs.rows.back();
+    row.block.size = 2;
+    row.block.position = 2;
+    row.cells.push_back(Cell(0, 0));
+    row.cells.push_back(Cell(2, 0));
+  }
+
+  int row_block_size = 0;
+  int e_block_size = 0;
+  int f_block_size = 0;
+  const int num_eliminate_blocks = 1;
+  DetectStructure(bs,
+                  num_eliminate_blocks,
+                  &row_block_size,
+                  &e_block_size,
+                  &f_block_size);
+
+  EXPECT_EQ(row_block_size, expected_row_block_size);
+  EXPECT_EQ(e_block_size, expected_e_block_size);
+  EXPECT_EQ(f_block_size, expected_f_block_size);
+}
+
+TEST(DetectStructure, DynamicRow) {
+  const int expected_row_block_size = Eigen::Dynamic;
+  const int expected_e_block_size = 3;
+  const int expected_f_block_size = 4;
+
+  CompressedRowBlockStructure bs;
+
+  bs.cols.push_back(Block());
+  bs.cols.back().size = 3;
+  bs.cols.back().position = 0;
+
+  bs.cols.push_back(Block());
+  bs.cols.back().size = 4;
+  bs.cols.back().position = 3;
+
+  bs.cols.push_back(Block());
+  bs.cols.back().size = 4;
+  bs.cols.back().position = 7;
+
+  {
+    bs.rows.push_back(CompressedRow());
+    CompressedRow& row = bs.rows.back();
+    row.block.size = 2;
+    row.block.position = 0;
+    row.cells.push_back(Cell(0, 0));
+    row.cells.push_back(Cell(1, 0));
+  }
+
+  {
+    bs.rows.push_back(CompressedRow());
+    CompressedRow& row = bs.rows.back();
+    row.block.size = 1;
+    row.block.position = 2;
+    row.cells.push_back(Cell(0, 0));
+    row.cells.push_back(Cell(2, 0));
+  }
+
+  int row_block_size = 0;
+  int e_block_size = 0;
+  int f_block_size = 0;
+  const int num_eliminate_blocks = 1;
+  DetectStructure(bs,
+                  num_eliminate_blocks,
+                  &row_block_size,
+                  &e_block_size,
+                  &f_block_size);
+
+  EXPECT_EQ(row_block_size, expected_row_block_size);
+  EXPECT_EQ(e_block_size, expected_e_block_size);
+  EXPECT_EQ(f_block_size, expected_f_block_size);
+}
+
+TEST(DetectStructure, DynamicFBlockDifferentRows) {
+  const int expected_row_block_size = 2;
+  const int expected_e_block_size = 3;
+  const int expected_f_block_size = Eigen::Dynamic;
+
+
+  CompressedRowBlockStructure bs;
+
+  bs.cols.push_back(Block());
+  bs.cols.back().size = 3;
+  bs.cols.back().position = 0;
+
+  bs.cols.push_back(Block());
+  bs.cols.back().size = 4;
+  bs.cols.back().position = 3;
+
+  bs.cols.push_back(Block());
+  bs.cols.back().size = 3;
+  bs.cols.back().position = 7;
+
+  {
+    bs.rows.push_back(CompressedRow());
+    CompressedRow& row = bs.rows.back();
+    row.block.size = 2;
+    row.block.position = 0;
+    row.cells.push_back(Cell(0, 0));
+    row.cells.push_back(Cell(1, 0));
+  }
+
+  {
+    bs.rows.push_back(CompressedRow());
+    CompressedRow& row = bs.rows.back();
+    row.block.size = 2;
+    row.block.position = 2;
+    row.cells.push_back(Cell(0, 0));
+    row.cells.push_back(Cell(2, 0));
+  }
+
+  int row_block_size = 0;
+  int e_block_size = 0;
+  int f_block_size = 0;
+  const int num_eliminate_blocks = 1;
+  DetectStructure(bs,
+                  num_eliminate_blocks,
+                  &row_block_size,
+                  &e_block_size,
+                  &f_block_size);
+
+  EXPECT_EQ(row_block_size, expected_row_block_size);
+  EXPECT_EQ(e_block_size, expected_e_block_size);
+  EXPECT_EQ(f_block_size, expected_f_block_size);
+}
+
+TEST(DetectStructure, DynamicEBlock) {
+  const int expected_row_block_size = 2;
+  const int expected_e_block_size = Eigen::Dynamic;
+  const int expected_f_block_size = 3;
+
+  CompressedRowBlockStructure bs;
+
+  bs.cols.push_back(Block());
+  bs.cols.back().size = 3;
+  bs.cols.back().position = 0;
+
+  bs.cols.push_back(Block());
+  bs.cols.back().size = 4;
+  bs.cols.back().position = 3;
+
+  bs.cols.push_back(Block());
+  bs.cols.back().size = 3;
+  bs.cols.back().position = 7;
+
+  {
+    bs.rows.push_back(CompressedRow());
+    CompressedRow& row = bs.rows.back();
+    row.block.size = 2;
+    row.block.position = 0;
+    row.cells.push_back(Cell(0, 0));
+    row.cells.push_back(Cell(2, 0));
+  }
+
+  {
+    bs.rows.push_back(CompressedRow());
+    CompressedRow& row = bs.rows.back();
+    row.block.size = 2;
+    row.block.position = 2;
+    row.cells.push_back(Cell(1, 0));
+    row.cells.push_back(Cell(2, 0));
+  }
+
+  int row_block_size = 0;
+  int e_block_size = 0;
+  int f_block_size = 0;
+  const int num_eliminate_blocks = 2;
+  DetectStructure(bs,
+                  num_eliminate_blocks,
+                  &row_block_size,
+                  &e_block_size,
+                  &f_block_size);
+
+  EXPECT_EQ(row_block_size, expected_row_block_size);
+  EXPECT_EQ(e_block_size, expected_e_block_size);
+  EXPECT_EQ(f_block_size, expected_f_block_size);
+}
+
+TEST(DetectStructure, DynamicFBlockSameRow) {
+  const int expected_row_block_size = 2;
+  const int expected_e_block_size = 3;
+  const int expected_f_block_size = Eigen::Dynamic;
+
+  CompressedRowBlockStructure bs;
+
+  bs.cols.push_back(Block());
+  bs.cols.back().size = 3;
+  bs.cols.back().position = 0;
+
+  bs.cols.push_back(Block());
+  bs.cols.back().size = 4;
+  bs.cols.back().position = 3;
+
+  bs.cols.push_back(Block());
+  bs.cols.back().size = 3;
+  bs.cols.back().position = 7;
+
+  {
+    bs.rows.push_back(CompressedRow());
+    CompressedRow& row = bs.rows.back();
+    row.block.size = 2;
+    row.block.position = 0;
+    row.cells.push_back(Cell(0, 0));
+    row.cells.push_back(Cell(2, 0));
+    row.cells.push_back(Cell(3, 0));
+  }
+
+  int row_block_size = 0;
+  int e_block_size = 0;
+  int f_block_size = 0;
+  const int num_eliminate_blocks = 1;
+  DetectStructure(bs,
+                  num_eliminate_blocks,
+                  &row_block_size,
+                  &e_block_size,
+                  &f_block_size);
+
+  EXPECT_EQ(row_block_size, expected_row_block_size);
+  EXPECT_EQ(e_block_size, expected_e_block_size);
+  EXPECT_EQ(f_block_size, expected_f_block_size);
+}
+
+}  // namespace internal
+}  // namespace ceres