Minor refactoring/cleanup in IterativeSchurComplementSolver

The code for creating and updating preconditioners has been pulled
out in into its own function for better readability.

Change-Id: I5335de3b158a8485cf6d052ee37d0d8fd57145e9

internal/ceres/block_jacobi_preconditioner.h

@@ -31,7 +31,6 @@
 #ifndef CERES_INTERNAL_BLOCK_JACOBI_PRECONDITIONER_H_
 #define CERES_INTERNAL_BLOCK_JACOBI_PRECONDITIONER_H_
 
-#include <vector>
 #include "ceres/block_random_access_diagonal_matrix.h"
 #include "ceres/internal/scoped_ptr.h"
 #include "ceres/preconditioner.h"
@@ -62,8 +61,8 @@ class BlockJacobiPreconditioner : public BlockSparseMatrixPreconditioner {
   virtual void RightMultiply(const double* x, double* y) const;
   virtual int num_rows() const { return m_->num_rows(); }
   virtual int num_cols() const { return m_->num_rows(); }
-
   const BlockRandomAccessDiagonalMatrix& matrix() const { return *m_; }
+
  private:
   virtual bool UpdateImpl(const BlockSparseMatrix& A, const double* D);
 

internal/ceres/iterative_schur_complement_solver.cc

@@ -56,11 +56,9 @@ namespace internal {
 
 IterativeSchurComplementSolver::IterativeSchurComplementSolver(
     const LinearSolver::Options& options)
-    : options_(options) {
-}
+    : options_(options) {}
 
-IterativeSchurComplementSolver::~IterativeSchurComplementSolver() {
-}
+IterativeSchurComplementSolver::~IterativeSchurComplementSolver() {}
 
 LinearSolver::Summary IterativeSchurComplementSolver::SolveImpl(
     BlockSparseMatrix* A,
@@ -86,29 +84,59 @@ LinearSolver::Summary IterativeSchurComplementSolver::SolveImpl(
       A->block_structure()->cols.size() - num_eliminate_blocks;
   if (num_schur_complement_blocks == 0) {
     VLOG(2) << "No parameter blocks left in the schur complement.";
-    LinearSolver::Summary cg_summary;
-    cg_summary.num_iterations = 0;
-    cg_summary.termination_type = LINEAR_SOLVER_SUCCESS;
+    LinearSolver::Summary summary;
+    summary.num_iterations = 0;
+    summary.termination_type = LINEAR_SOLVER_SUCCESS;
     schur_complement_->BackSubstitute(NULL, x);
-    return cg_summary;
+    return summary;
   }
 
   // Initialize the solution to the Schur complement system to zero.
   reduced_linear_system_solution_.resize(schur_complement_->num_rows());
   reduced_linear_system_solution_.setZero();
 
-  // Instantiate a conjugate gradient solver that runs on the Schur
-  // complement matrix with the block diagonal of the matrix F'F as
-  // the preconditioner.
   LinearSolver::Options cg_options;
   cg_options.min_num_iterations = options_.min_num_iterations;
   cg_options.max_num_iterations = options_.max_num_iterations;
   ConjugateGradientsSolver cg_solver(cg_options);
-  LinearSolver::PerSolveOptions cg_per_solve_options;
 
+  LinearSolver::PerSolveOptions cg_per_solve_options;
   cg_per_solve_options.r_tolerance = per_solve_options.r_tolerance;
   cg_per_solve_options.q_tolerance = per_solve_options.q_tolerance;
 
+  if (!CreateAndOrUpdatePreconditioner(A, per_solve_options.D)) {
+    LinearSolver::Summary summary;
+    summary.num_iterations = 0;
+    summary.termination_type = LINEAR_SOLVER_FAILURE;
+    summary.message = "Preconditioner creation or update failed.";
+    return summary;
+  }
+
+  if (preconditioner_.get() != NULL) {
+    cg_per_solve_options.preconditioner = preconditioner_.get();
+  }
+
+  event_logger.AddEvent("Setup");
+  LinearSolver::Summary summary =
+      cg_solver.Solve(schur_complement_.get(),
+                      schur_complement_->rhs().data(),
+                      cg_per_solve_options,
+                      reduced_linear_system_solution_.data());
+  if (summary.termination_type != LINEAR_SOLVER_FAILURE &&
+      summary.termination_type != LINEAR_SOLVER_FATAL_ERROR) {
+    schur_complement_->BackSubstitute(reduced_linear_system_solution_.data(),
+                                      x);
+  }
+  event_logger.AddEvent("Solve");
+  return summary;
+}
+
+bool IterativeSchurComplementSolver::CreateAndOrUpdatePreconditioner(
+    BlockSparseMatrix* A, double* D) {
+  if (options_.preconditioner_type == IDENTITY) {
+    return true;
+  }
+
   Preconditioner::Options preconditioner_options;
   preconditioner_options.type = options_.preconditioner_type;
   preconditioner_options.visibility_clustering_type =
@@ -121,61 +149,27 @@ LinearSolver::Summary IterativeSchurComplementSolver::SolveImpl(
   preconditioner_options.f_block_size = options_.f_block_size;
   preconditioner_options.elimination_groups = options_.elimination_groups;
 
-  switch (options_.preconditioner_type) {
-    case IDENTITY:
-      break;
-    case JACOBI:
-      preconditioner_.reset(
-          new SparseMatrixPreconditionerWrapper(
-              schur_complement_->block_diagonal_FtF_inverse()));
-      break;
-    case SCHUR_JACOBI:
-      if (preconditioner_.get() == NULL) {
-        preconditioner_.reset(
-            new SchurJacobiPreconditioner(*A->block_structure(),
-                                          preconditioner_options));
-      }
-      break;
-    case CLUSTER_JACOBI:
-    case CLUSTER_TRIDIAGONAL:
-      if (preconditioner_.get() == NULL) {
-        preconditioner_.reset(
-            new VisibilityBasedPreconditioner(*A->block_structure(),
-                                              preconditioner_options));
-      }
-      break;
-    default:
-      LOG(FATAL) << "Unknown Preconditioner Type";
-  }
-
-  bool preconditioner_update_was_successful = true;
-  if (preconditioner_.get() != NULL) {
-    preconditioner_update_was_successful =
-        preconditioner_->Update(*A, per_solve_options.D);
-    cg_per_solve_options.preconditioner = preconditioner_.get();
-  }
-  event_logger.AddEvent("Setup");
-
-  LinearSolver::Summary cg_summary;
-  cg_summary.num_iterations = 0;
-  cg_summary.termination_type = LINEAR_SOLVER_FAILURE;
-
-  // TODO(sameeragarwal): Refactor preconditioners to return a more
-  // sane message.
-  cg_summary.message = "Preconditioner update failed.";
-  if (preconditioner_update_was_successful) {
-    cg_summary = cg_solver.Solve(schur_complement_.get(),
-                                 schur_complement_->rhs().data(),
-                                 cg_per_solve_options,
-                                 reduced_linear_system_solution_.data());
-    if (cg_summary.termination_type != LINEAR_SOLVER_FAILURE &&
-        cg_summary.termination_type != LINEAR_SOLVER_FATAL_ERROR) {
-      schur_complement_->BackSubstitute(
-          reduced_linear_system_solution_.data(), x);
+  if (preconditioner_.get() == NULL) {
+    switch (options_.preconditioner_type) {
+      case JACOBI:
+        preconditioner_.reset(new SparseMatrixPreconditionerWrapper(
+            schur_complement_->block_diagonal_FtF_inverse()));
+        break;
+      case SCHUR_JACOBI:
+        preconditioner_.reset(new SchurJacobiPreconditioner(
+            *A->block_structure(), preconditioner_options));
+        break;
+      case CLUSTER_JACOBI:
+      case CLUSTER_TRIDIAGONAL:
+        preconditioner_.reset(new VisibilityBasedPreconditioner(
+            *A->block_structure(), preconditioner_options));
+        break;
+      default:
+        LOG(FATAL) << "Unknown Preconditioner Type";
     }
   }
-  event_logger.AddEvent("Solve");
-  return cg_summary;
+
+  return preconditioner_->Update(*A, D);
 }
 
 }  // namespace internal

internal/ceres/iterative_schur_complement_solver.h

@@ -79,6 +79,8 @@ class IterativeSchurComplementSolver : public BlockSparseMatrixSolver {
       const LinearSolver::PerSolveOptions& options,
       double* x);
 
+  bool CreateAndOrUpdatePreconditioner(BlockSparseMatrix* A, double* D);
+
   LinearSolver::Options options_;
   scoped_ptr<internal::ImplicitSchurComplement> schur_complement_;
   scoped_ptr<Preconditioner> preconditioner_;