Fix a bug in InnerProductComputer. InnerProductComputer was assuming that the input matrix had at least one structurally non-zero block. As a result sometimes when InnerProductComputer.SubMatrix test generated matrices where the submatrix was structurally zero it would cause a segmentation fault. This changes fixes this issue and reduces the threshold for the minimum block density so that there is a much higher chance of such matrices occuring as part of testing. Fixes https://github.com/ceres-solver/ceres-solver/issues/820 Change-Id: Iec3a2431e646f0c2aac9e0b994531fa72323f329
diff --git a/internal/ceres/inner_product_computer.cc b/internal/ceres/inner_product_computer.cc index 69a68ee..a75cbe5 100644 --- a/internal/ceres/inner_product_computer.cc +++ b/internal/ceres/inner_product_computer.cc
@@ -77,6 +77,10 @@ row_nnz->resize(blocks.size()); std::fill(row_nnz->begin(), row_nnz->end(), 0); + if (product_terms.empty()) { + return 0; + } + // First product term. (*row_nnz)[product_terms[0].row] = blocks[product_terms[0].col].size; int num_nonzeros = @@ -197,6 +201,10 @@ *(crsm_rows + 1) = *crsm_rows + row_block_nnz[i]; } } + result_offsets_.resize(product_terms.size()); + if (num_nonzeros == 0) { + return; + } // The following macro FILL_CRSM_COL_BLOCK is key to understanding // how this class works. @@ -243,7 +251,6 @@ } \ } - result_offsets_.resize(product_terms.size()); int col_nnz = 0; int nnz = 0;
diff --git a/internal/ceres/inner_product_computer_test.cc b/internal/ceres/inner_product_computer_test.cc index ba29da3..a1f5c49 100644 --- a/internal/ceres/inner_product_computer_test.cc +++ b/internal/ceres/inner_product_computer_test.cc
@@ -44,36 +44,36 @@ namespace ceres { namespace internal { -#define COMPUTE_AND_COMPARE \ - { \ - inner_product_computer->Compute(); \ - CompressedRowSparseMatrix* actual_product_crsm = \ - inner_product_computer->mutable_result(); \ - Matrix actual_inner_product = \ - Eigen::Map<Eigen::SparseMatrix<double, Eigen::ColMajor>>( \ - actual_product_crsm->num_rows(), \ - actual_product_crsm->num_rows(), \ - actual_product_crsm->num_nonzeros(), \ - actual_product_crsm->mutable_rows(), \ - actual_product_crsm->mutable_cols(), \ - actual_product_crsm->mutable_values()); \ - EXPECT_EQ(actual_inner_product.rows(), actual_inner_product.cols()); \ - EXPECT_EQ(expected_inner_product.rows(), expected_inner_product.cols()); \ - EXPECT_EQ(actual_inner_product.rows(), expected_inner_product.rows()); \ - Matrix expected_t, actual_t; \ - if (actual_product_crsm->storage_type() == \ - CompressedRowSparseMatrix::StorageType::LOWER_TRIANGULAR) { \ - expected_t = expected_inner_product.triangularView<Eigen::Upper>(); \ - actual_t = actual_inner_product.triangularView<Eigen::Upper>(); \ - } else { \ - expected_t = expected_inner_product.triangularView<Eigen::Lower>(); \ - actual_t = actual_inner_product.triangularView<Eigen::Lower>(); \ - } \ - EXPECT_LE((expected_t - actual_t).norm() / actual_t.norm(), \ - 100 * std::numeric_limits<double>::epsilon()) \ - << "expected: \n" \ - << expected_t << "\nactual: \n" \ - << actual_t; \ +#define COMPUTE_AND_COMPARE \ + { \ + inner_product_computer->Compute(); \ + CompressedRowSparseMatrix* actual_product_crsm = \ + inner_product_computer->mutable_result(); \ + Matrix actual_inner_product = \ + Eigen::Map<Eigen::SparseMatrix<double, Eigen::ColMajor>>( \ + actual_product_crsm->num_rows(), \ + actual_product_crsm->num_rows(), \ + actual_product_crsm->num_nonzeros(), \ + actual_product_crsm->mutable_rows(), \ + actual_product_crsm->mutable_cols(), \ + actual_product_crsm->mutable_values()); \ + EXPECT_EQ(actual_inner_product.rows(), actual_inner_product.cols()); \ + EXPECT_EQ(expected_inner_product.rows(), expected_inner_product.cols()); \ + EXPECT_EQ(actual_inner_product.rows(), expected_inner_product.rows()); \ + Matrix expected_t, actual_t; \ + if (actual_product_crsm->storage_type() == \ + CompressedRowSparseMatrix::StorageType::LOWER_TRIANGULAR) { \ + expected_t = expected_inner_product.triangularView<Eigen::Upper>(); \ + actual_t = actual_inner_product.triangularView<Eigen::Upper>(); \ + } else { \ + expected_t = expected_inner_product.triangularView<Eigen::Lower>(); \ + actual_t = actual_inner_product.triangularView<Eigen::Lower>(); \ + } \ + EXPECT_LE((expected_t - actual_t).norm(), \ + 100 * std::numeric_limits<double>::epsilon() * actual_t.norm()) \ + << "expected: \n" \ + << expected_t << "\nactual: \n" \ + << actual_t; \ } TEST(InnerProductComputer, NormalOperation) { @@ -99,7 +99,6 @@ options.min_col_block_size = 1; options.max_col_block_size = 10; options.block_density = std::max(0.1, RandDouble()); - VLOG(2) << "num row blocks: " << options.num_row_blocks; VLOG(2) << "num col blocks: " << options.num_col_blocks; VLOG(2) << "min row block size: " << options.min_row_block_size; @@ -141,10 +140,6 @@ } TEST(InnerProductComputer, SubMatrix) { - // "Randomly generated seed." - // FIXME Use a seed different from the previous one to allow the test to pass - // without a segmentation fault when compiled using 32 bit MinGW. - SetRandomState(29824); const int kNumRowBlocks = 10; const int kNumColBlocks = 20; const int kNumTrials = 5; @@ -160,7 +155,7 @@ options.max_row_block_size = 5; options.min_col_block_size = 1; options.max_col_block_size = 10; - options.block_density = std::max(0.1, RandDouble()); + options.block_density = std::min(0.01, RandDouble()); VLOG(2) << "num row blocks: " << options.num_row_blocks; VLOG(2) << "num col blocks: " << options.num_col_blocks;