From beb4505311011130a7e54632137b0fbb5824cc9b Mon Sep 17 00:00:00 2001 From: Sameer Agarwal Date: Fri, 22 Feb 2013 13:37:01 -0800 Subject: [PATCH] Minor fixes Based on William Rucklidge's review, including a nasty bug in parameter block removal. Change-Id: I3a692e589f600ff560ecae9fa85bb0b76063d403 --- include/ceres/rotation.h | 20 ++++++++++++------- internal/ceres/block_jacobi_preconditioner.h | 2 +- .../iterative_schur_complement_solver.cc | 6 ------ internal/ceres/parameter_block.h | 2 +- internal/ceres/preconditioner.h | 12 +++++------ internal/ceres/problem_impl.cc | 6 +++--- internal/ceres/solver.cc | 2 +- internal/ceres/stl_util.h | 2 ++ scripts/make_docs.py | 8 +++++--- 9 files changed, 32 insertions(+), 28 deletions(-) diff --git a/include/ceres/rotation.h b/include/ceres/rotation.h index 74cbcf148..711687f74 100644 --- a/include/ceres/rotation.h +++ b/include/ceres/rotation.h @@ -51,13 +51,19 @@ namespace ceres { -// Trivial wrapper to index linear arrays as matrices, given a fixed column and -// row stride. When an array "T* arr" is wrapped by a -// "(const) MatrixAdapter M", the expression "M(i, j)" is -// equivalent to "arr[i * row_stride + j * col_stride]". -// Conversion functions to and from rotation matrices accept MatrixAdapters to -// permit using row-major and column-major layouts, and rotation matrices embedded in -// larger matrices (such as a 3x4 projection matrix). +// Trivial wrapper to index linear arrays as matrices, given a fixed +// column and row stride. When an array "T* array" is wrapped by a +// +// (const) MatrixAdapter M" +// +// the expression M(i, j) is equivalent to +// +// arrary[i * row_stride + j * col_stride] +// +// Conversion functions to and from rotation matrices accept +// MatrixAdapters to permit using row-major and column-major layouts, +// and rotation matrices embedded in larger matrices (such as a 3x4 +// projection matrix). template struct MatrixAdapter; diff --git a/internal/ceres/block_jacobi_preconditioner.h b/internal/ceres/block_jacobi_preconditioner.h index 32221f5a2..ed5eebc8d 100644 --- a/internal/ceres/block_jacobi_preconditioner.h +++ b/internal/ceres/block_jacobi_preconditioner.h @@ -54,7 +54,7 @@ class LinearOperator; class BlockJacobiPreconditioner : public Preconditioner { public: // A must remain valid while the BlockJacobiPreconditioner is. - BlockJacobiPreconditioner(const BlockSparseMatrixBase& A); + explicit BlockJacobiPreconditioner(const BlockSparseMatrixBase& A); virtual ~BlockJacobiPreconditioner(); // Preconditioner interface diff --git a/internal/ceres/iterative_schur_complement_solver.cc b/internal/ceres/iterative_schur_complement_solver.cc index c44bd3921..d352d023b 100644 --- a/internal/ceres/iterative_schur_complement_solver.cc +++ b/internal/ceres/iterative_schur_complement_solver.cc @@ -110,12 +110,6 @@ LinearSolver::Summary IterativeSchurComplementSolver::SolveImpl( case IDENTITY: break; case JACOBI: - // We need to strip the constness of the block_diagonal_FtF_inverse - // matrix here because the only other way to initialize the struct - // cg_solve_options would be to add a constructor to it. We know - // that the only method ever called on the preconditioner is the - // RightMultiply which is a const method so we don't need to worry - // about the object getting modified. preconditioner_.reset( new SparseMatrixPreconditionerWrapper( schur_complement_->block_diagonal_FtF_inverse())); diff --git a/internal/ceres/parameter_block.h b/internal/ceres/parameter_block.h index 3a709f4c6..b1e8d938b 100644 --- a/internal/ceres/parameter_block.h +++ b/internal/ceres/parameter_block.h @@ -73,7 +73,7 @@ class ParameterBlock { // Create a parameter block with the user state, size, and index specified. // The size is the size of the parameter block and the index is the position - // if the parameter block inside a Program (if any). + // of the parameter block inside a Program (if any). ParameterBlock(double* user_state, int size, int index) { Init(user_state, size, index, NULL); } diff --git a/internal/ceres/preconditioner.h b/internal/ceres/preconditioner.h index 77280c3e1..5bb077e0e 100644 --- a/internal/ceres/preconditioner.h +++ b/internal/ceres/preconditioner.h @@ -69,11 +69,11 @@ class Preconditioner : public LinearOperator { // // For example if elimination_groups is a vector of size k, then // the linear solver is informed that it should eliminate the - // parameter blocks 0 - elimination_groups[0] - 1 first, and then - // elimination_groups[0] - elimination_groups[1] and so on. Within - // each elimination group, the linear solver is free to choose how - // the parameter blocks are ordered. Different linear solvers have - // differing requirements on elimination_groups. + // parameter blocks 0 ... elimination_groups[0] - 1 first, and + // then elimination_groups[0] ... elimination_groups[1] and so + // on. Within each elimination group, the linear solver is free to + // choose how the parameter blocks are ordered. Different linear + // solvers have differing requirements on elimination_groups. // // The most common use is for Schur type solvers, where there // should be at least two elimination groups and the first @@ -130,7 +130,7 @@ class Preconditioner : public LinearOperator { class SparseMatrixPreconditionerWrapper : public Preconditioner { public: // Wrapper does NOT take ownership of the matrix pointer. - SparseMatrixPreconditionerWrapper(const SparseMatrix* matrix); + explicit SparseMatrixPreconditionerWrapper(const SparseMatrix* matrix); virtual ~SparseMatrixPreconditionerWrapper(); // Preconditioner interface diff --git a/internal/ceres/problem_impl.cc b/internal/ceres/problem_impl.cc index 6154ddf65..ae4e7ed75 100644 --- a/internal/ceres/problem_impl.cc +++ b/internal/ceres/problem_impl.cc @@ -177,7 +177,7 @@ ProblemImpl::ProblemImpl(const Problem::Options& options) ProblemImpl::~ProblemImpl() { // Collect the unique cost/loss functions and delete the residuals. - const int num_residual_blocks = program_->residual_blocks_.size(); + const int num_residual_blocks = program_->residual_blocks_.size(); cost_functions_to_delete_.reserve(num_residual_blocks); loss_functions_to_delete_.reserve(num_residual_blocks); for (int i = 0; i < program_->residual_blocks_.size(); ++i) { @@ -486,8 +486,8 @@ void ProblemImpl::RemoveParameterBlock(double* values) { ResidualBlock* residual_block = (*(program_->mutable_residual_blocks()))[i]; const int num_parameter_blocks = residual_block->NumParameterBlocks(); - for (int i = 0; i < num_parameter_blocks; ++i) { - if (residual_block->parameter_blocks()[i] == parameter_block) { + for (int j = 0; j < num_parameter_blocks; ++j) { + if (residual_block->parameter_blocks()[j] == parameter_block) { RemoveResidualBlock(residual_block); // The parameter blocks are guaranteed unique. break; diff --git a/internal/ceres/solver.cc b/internal/ceres/solver.cc index 6d38535da..b1bd2d82a 100644 --- a/internal/ceres/solver.cc +++ b/internal/ceres/solver.cc @@ -311,7 +311,7 @@ string Solver::Summary::FullReport() const { StringAppendF(&report, "\n"); - StringAppendF(&report, "%45s %21s\n", "Given", "Used"); + StringAppendF(&report, "%45s %21s\n", "Given", "Used"); StringAppendF(&report, "Threads: % 25d% 25d\n", num_threads_given, num_threads_used); diff --git a/internal/ceres/stl_util.h b/internal/ceres/stl_util.h index 5e92eff9f..b23e7c12e 100644 --- a/internal/ceres/stl_util.h +++ b/internal/ceres/stl_util.h @@ -53,6 +53,8 @@ void STLDeleteContainerPointers(ForwardIterator begin, } } +// Variant of STLDeleteContainerPointers which allows the container to +// contain duplicates. template void STLDeleteUniqueContainerPointers(ForwardIterator begin, ForwardIterator end) { diff --git a/scripts/make_docs.py b/scripts/make_docs.py index 5bffd5be3..41879cb6d 100644 --- a/scripts/make_docs.py +++ b/scripts/make_docs.py @@ -38,7 +38,7 @@ import glob if len(sys.argv) < 3: print "make_docs.py src_root destination_root" - sys.exit(1); + sys.exit(1) src_dir = sys.argv[1] + "/docs/source" build_root = sys.argv[2] @@ -58,8 +58,10 @@ output_pattern = """config=TeX-AMS_HTML"> """ # By default MathJax uses does not use TeX fonts. This simple search -# an replace fixes that. +# and replace fixes that. for name in glob.glob("%s/*.html" % html_dir): print "Postprocessing: ", name - out = open(name).read().replace(input_pattern, output_pattern) + fptr = open(name) + out = fptr.read().replace(input_pattern, output_pattern) open(name, "w").write(out) + fptr.close();