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
diff --git a/internal/ceres/CMakeLists.txt b/internal/ceres/CMakeLists.txt
index 118f2e0..a69fa49 100644
--- a/internal/ceres/CMakeLists.txt
+++ b/internal/ceres/CMakeLists.txt
@@ -273,6 +273,7 @@
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)
diff --git a/internal/ceres/detect_structure.cc b/internal/ceres/detect_structure.cc
index a87f6ba6..959a0ee 100644
--- a/internal/ceres/detect_structure.cc
+++ b/internal/ceres/detect_structure.cc
@@ -55,6 +55,7 @@
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 @@
*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 @@
*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;
}
}
}
diff --git a/internal/ceres/detect_structure_test.cc b/internal/ceres/detect_structure_test.cc
new file mode 100644
index 0000000..533f49f
--- /dev/null
+++ b/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
