SuiteSparse cleanup. 1. Silence CHOLMOD's indefiniteness warnings. 2. Add a comment about how the error handling in suitesparse.cc needs to be improved. 3. Move the analysis logging into suitesparse.cc and out of the three callsites. Change-Id: Idd396b8ea4bf59fc1ffc7f9fcbbc7b38ed71643c
diff --git a/internal/ceres/schur_complement_solver.cc b/internal/ceres/schur_complement_solver.cc index 93626bc..875f455 100644 --- a/internal/ceres/schur_complement_solver.cc +++ b/internal/ceres/schur_complement_solver.cc
@@ -291,15 +291,8 @@ } else { factor_ = ss_.AnalyzeCholesky(cholmod_lhs); } - - if (VLOG_IS_ON(2)) { - cholmod_print_common(const_cast<char*>("Symbolic Analysis"), - ss_.mutable_cc()); - } } - CHECK_NOTNULL(factor_); - cholmod_dense* cholmod_solution = ss_.SolveCholesky(cholmod_lhs, factor_, cholmod_rhs);
diff --git a/internal/ceres/sparse_normal_cholesky_solver.cc b/internal/ceres/sparse_normal_cholesky_solver.cc index d2a7635..aacba86 100644 --- a/internal/ceres/sparse_normal_cholesky_solver.cc +++ b/internal/ceres/sparse_normal_cholesky_solver.cc
@@ -213,14 +213,7 @@ } else { factor_ = ss_.AnalyzeCholesky(lhs.get()); } - - if (VLOG_IS_ON(2)) { - cholmod_print_common(const_cast<char*>("Symbolic Analysis"), - ss_.mutable_cc()); - } } - - CHECK_NOTNULL(factor_); event_logger.AddEvent("Analysis"); cholmod_dense* sol = ss_.SolveCholesky(lhs.get(), factor_, rhs);
diff --git a/internal/ceres/suitesparse.cc b/internal/ceres/suitesparse.cc index d200aeb..87fae75 100644 --- a/internal/ceres/suitesparse.cc +++ b/internal/ceres/suitesparse.cc
@@ -35,8 +35,18 @@ #include "cholmod.h" #include "ceres/compressed_row_sparse_matrix.h" #include "ceres/triplet_sparse_matrix.h" + namespace ceres { namespace internal { + +SuiteSparse::SuiteSparse() { + cholmod_start(&cc_); +} + +SuiteSparse::~SuiteSparse() { + cholmod_finish(&cc_); +} + cholmod_sparse* SuiteSparse::CreateSparseMatrix(TripletSparseMatrix* A) { cholmod_triplet triplet; @@ -117,10 +127,16 @@ cc_.nmethods = 1; cc_.method[0].ordering = CHOLMOD_AMD; cc_.supernodal = CHOLMOD_AUTO; + cholmod_factor* factor = cholmod_analyze(A, &cc_); CHECK_EQ(cc_.status, CHOLMOD_OK) << "Cholmod symbolic analysis failed " << cc_.status; CHECK_NOTNULL(factor); + + if (VLOG_IS_ON(2)) { + cholmod_print_common(const_cast<char*>("Symbolic Analysis"), &cc_); + } + return factor; } @@ -139,13 +155,20 @@ cholmod_sparse* A, const vector<int>& ordering) { CHECK_EQ(ordering.size(), A->nrow); + cc_.nmethods = 1; cc_.method[0].ordering = CHOLMOD_GIVEN; + cholmod_factor* factor = cholmod_analyze_p(A, const_cast<int*>(&ordering[0]), NULL, 0, &cc_); CHECK_EQ(cc_.status, CHOLMOD_OK) << "Cholmod symbolic analysis failed " << cc_.status; CHECK_NOTNULL(factor); + + if (VLOG_IS_ON(2)) { + cholmod_print_common(const_cast<char*>("Symbolic Analysis"), &cc_); + } + return factor; } @@ -276,36 +299,52 @@ CHECK_NOTNULL(A); CHECK_NOTNULL(L); + // Save the current print level and silence CHOLMOD, otherwise + // CHOLMOD is prone to dumping stuff to stderr, which can be + // distracting when the error (matrix is indefinite) is not a fatal + // failure. + const int old_print_level = cc_.print; + cc_.print = 0; + cc_.quick_return_if_not_posdef = 1; int status = cholmod_factorize(A, L, &cc_); + cc_.print = old_print_level; + + // TODO(sameeragarwal): This switch statement is not consistent. It + // treats all kinds of CHOLMOD failures as warnings. Some of these + // like out of memory are definitely not warnings. The problem is + // that the return value Cholesky is two valued, but the state of + // the linear solver is really three valued. SUCCESS, + // NON_FATAL_FAILURE (e.g., indefinite matrix) and FATAL_FAILURE + // (e.g. out of memory). switch (cc_.status) { case CHOLMOD_NOT_INSTALLED: - LOG(WARNING) << "Cholmod failure: method not installed."; + LOG(WARNING) << "CHOLMOD failure: Method not installed."; return false; case CHOLMOD_OUT_OF_MEMORY: - LOG(WARNING) << "Cholmod failure: out of memory."; + LOG(WARNING) << "CHOLMOD failure: Out of memory."; return false; case CHOLMOD_TOO_LARGE: - LOG(WARNING) << "Cholmod failure: integer overflow occured."; + LOG(WARNING) << "CHOLMOD failure: Integer overflow occured."; return false; case CHOLMOD_INVALID: - LOG(WARNING) << "Cholmod failure: invalid input."; + LOG(WARNING) << "CHOLMOD failure: Invalid input."; return false; case CHOLMOD_NOT_POSDEF: // TODO(sameeragarwal): These two warnings require more // sophisticated handling going forward. For now we will be // strict and treat them as failures. - LOG(WARNING) << "Cholmod warning: matrix not positive definite."; + LOG(WARNING) << "CHOLMOD warning: Matrix not positive definite."; return false; case CHOLMOD_DSMALL: - LOG(WARNING) << "Cholmod warning: D for LDL' or diag(L) or " + LOG(WARNING) << "CHOLMOD warning: D for LDL' or diag(L) or " << "LL' has tiny absolute value."; return false; case CHOLMOD_OK: if (status != 0) { return true; } - LOG(WARNING) << "Cholmod failure: cholmod_factorize returned zero " + LOG(WARNING) << "CHOLMOD failure: cholmod_factorize returned zero " << "but cholmod_common::status is CHOLMOD_OK." << "Please report this to ceres-solver@googlegroups.com."; return false;
diff --git a/internal/ceres/suitesparse.h b/internal/ceres/suitesparse.h index 3fe7908..5a8ef63 100644 --- a/internal/ceres/suitesparse.h +++ b/internal/ceres/suitesparse.h
@@ -56,8 +56,8 @@ // for all cholmod function calls. class SuiteSparse { public: - SuiteSparse() { cholmod_start(&cc_); } - ~SuiteSparse() { cholmod_finish(&cc_); } + SuiteSparse(); + ~SuiteSparse(); // Functions for building cholmod_sparse objects from sparse // matrices stored in triplet form. The matrix A is not
diff --git a/internal/ceres/visibility_based_preconditioner.cc b/internal/ceres/visibility_based_preconditioner.cc index c1898c6..f5c46bf 100644 --- a/internal/ceres/visibility_based_preconditioner.cc +++ b/internal/ceres/visibility_based_preconditioner.cc
@@ -426,15 +426,8 @@ } else { factor_ = ss_.AnalyzeCholesky(lhs); } - - if (VLOG_IS_ON(2)) { - cholmod_print_common(const_cast<char*>("Symbolic Analysis"), - ss_.mutable_cc()); - } } - CHECK_NOTNULL(factor_); - bool status = ss_.Cholesky(lhs, factor_); ss_.Free(lhs); return status;