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