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;