From b4a5f7ea255df284100f2be0986e5cdee9adf66f Mon Sep 17 00:00:00 2001 From: Sameer Agarwal Date: Sun, 1 Jun 2014 14:47:22 -0700 Subject: [PATCH] Program::RemoveFixedBlocks -> Program::CreateReducedProgram. CreateReducedProgram is a safer API, reduces the possibility of memory leaks and produces valid programs. Change-Id: I094d53d207fced970a4d9ea0b66cdb09ce5f0657 --- internal/ceres/program.cc | 18 +++++++ internal/ceres/program.h | 39 +++++++++++---- internal/ceres/program_test.cc | 91 +++++++++++++++++++--------------- internal/ceres/solver_impl.cc | 62 ++++++++++++----------- 4 files changed, 132 insertions(+), 78 deletions(-) diff --git a/internal/ceres/program.cc b/internal/ceres/program.cc index 3a88b1336..1d0a1573e 100644 --- a/internal/ceres/program.cc +++ b/internal/ceres/program.cc @@ -261,6 +261,24 @@ bool Program::IsFeasible(string* message) const { return true; } +Program* Program::CreateReducedProgram(vector* removed_parameter_blocks, + double* fixed_cost, + string* error) const { + CHECK_NOTNULL(removed_parameter_blocks); + CHECK_NOTNULL(fixed_cost); + CHECK_NOTNULL(error); + + scoped_ptr reduced_program(new Program(*this)); + if (!reduced_program->RemoveFixedBlocks(removed_parameter_blocks, + fixed_cost, + error)) { + return NULL; + } + + reduced_program->SetParameterOffsetsAndIndex(); + return reduced_program.release(); +} + bool Program::RemoveFixedBlocks(vector* removed_parameter_blocks, double* fixed_cost, string* error) { diff --git a/internal/ceres/program.h b/internal/ceres/program.h index 6b9190d12..c7b22c4d2 100644 --- a/internal/ceres/program.h +++ b/internal/ceres/program.h @@ -129,16 +129,22 @@ class Program { // Caller owns the result. TripletSparseMatrix* CreateJacobianBlockSparsityTranspose() const; - // Removes constant parameter blocks and residual blocks with no - // varying parameter blocks while preserving order. + // Create a copy of this program and removes constant parameter + // blocks and residual blocks with no varying parameter blocks while + // preserving their relative order. // - // WARNING: It is the caller's responsibility to track ownership of - // the parameter and residual blocks removed from the - // program. Generally speaking this method should only be called on - // a copy of an existing program. - bool RemoveFixedBlocks(vector* removed_parameter_blocks, - double* fixed_cost, - string* message); + // removed_parameter_blocks on exit will contain the list of + // parameter blocks that were removed. + // + // fixed_cost will be equal to the sum of the costs of the residual + // blocks that were removed. + // + // If there was a problem, then the function will return a NULL + // pointer and error will contain a human readable description of + // the problem. + Program* CreateReducedProgram(vector* removed_parameter_blocks, + double* fixed_cost, + string* error) const; // See problem.h for what these do. int NumParameterBlocks() const; @@ -157,6 +163,21 @@ class Program { string ToString() const; private: + // Remove constant parameter blocks and residual blocks with no + // varying parameter blocks while preserving their relative order. + // + // removed_parameter_blocks on exit will contain the list of + // parameter blocks that were removed. + // + // fixed_cost will be equal to the sum of the costs of the residual + // blocks that were removed. + // + // If there was a problem, then the function will return false and + // error will contain a human readable description of the problem. + bool RemoveFixedBlocks(vector* removed_parameter_blocks, + double* fixed_cost, + string* message); + // The Program does not own the ParameterBlock or ResidualBlock objects. vector parameter_blocks_; vector residual_blocks_; diff --git a/internal/ceres/program_test.cc b/internal/ceres/program_test.cc index f273872ba..10bfa1296 100644 --- a/internal/ceres/program_test.cc +++ b/internal/ceres/program_test.cc @@ -88,15 +88,18 @@ TEST(Program, RemoveFixedBlocksNothingConstant) { problem.AddResidualBlock(new BinaryCostFunction(), NULL, &x, &y); problem.AddResidualBlock(new TernaryCostFunction(), NULL, &x, &y, &z); - Program program(problem.program()); vector removed_parameter_blocks; double fixed_cost = 0.0; string message; - EXPECT_TRUE(program.RemoveFixedBlocks(&removed_parameter_blocks, - &fixed_cost, - &message)); - EXPECT_EQ(program.NumParameterBlocks(), 3); - EXPECT_EQ(program.NumResidualBlocks(), 3); + scoped_ptr reduced_program( + CHECK_NOTNULL(problem + .program() + .CreateReducedProgram(&removed_parameter_blocks, + &fixed_cost, + &message))); + + EXPECT_EQ(reduced_program->NumParameterBlocks(), 3); + EXPECT_EQ(reduced_program->NumResidualBlocks(), 3); EXPECT_EQ(removed_parameter_blocks.size(), 0); EXPECT_EQ(fixed_cost, 0.0); } @@ -109,15 +112,17 @@ TEST(Program, RemoveFixedBlocksAllParameterBlocksConstant) { problem.AddResidualBlock(new UnaryCostFunction(), NULL, &x); problem.SetParameterBlockConstant(&x); - Program program(problem.program()); vector removed_parameter_blocks; double fixed_cost = 0.0; string message; - EXPECT_TRUE(program.RemoveFixedBlocks(&removed_parameter_blocks, - &fixed_cost, - &message)); - EXPECT_EQ(program.NumParameterBlocks(), 0); - EXPECT_EQ(program.NumResidualBlocks(), 0); + scoped_ptr reduced_program( + CHECK_NOTNULL(problem + .program() + .CreateReducedProgram(&removed_parameter_blocks, + &fixed_cost, + &message))); + EXPECT_EQ(reduced_program->NumParameterBlocks(), 0); + EXPECT_EQ(reduced_program->NumResidualBlocks(), 0); EXPECT_EQ(removed_parameter_blocks.size(), 1); EXPECT_EQ(removed_parameter_blocks[0], &x); EXPECT_EQ(fixed_cost, 9.0); @@ -134,15 +139,17 @@ TEST(Program, RemoveFixedBlocksNoResidualBlocks) { problem.AddParameterBlock(&y, 1); problem.AddParameterBlock(&z, 1); - Program program(problem.program()); vector removed_parameter_blocks; double fixed_cost = 0.0; string message; - EXPECT_TRUE(program.RemoveFixedBlocks(&removed_parameter_blocks, - &fixed_cost, - &message)); - EXPECT_EQ(program.NumParameterBlocks(), 0); - EXPECT_EQ(program.NumResidualBlocks(), 0); + scoped_ptr reduced_program( + CHECK_NOTNULL(problem + .program() + .CreateReducedProgram(&removed_parameter_blocks, + &fixed_cost, + &message))); + EXPECT_EQ(reduced_program->NumParameterBlocks(), 0); + EXPECT_EQ(reduced_program->NumResidualBlocks(), 0); EXPECT_EQ(removed_parameter_blocks.size(), 3); EXPECT_EQ(fixed_cost, 0.0); } @@ -161,15 +168,17 @@ TEST(Program, RemoveFixedBlocksOneParameterBlockConstant) { problem.AddResidualBlock(new BinaryCostFunction(), NULL, &x, &y); problem.SetParameterBlockConstant(&x); - Program program(problem.program()); vector removed_parameter_blocks; double fixed_cost = 0.0; string message; - EXPECT_TRUE(program.RemoveFixedBlocks(&removed_parameter_blocks, - &fixed_cost, - &message)); - EXPECT_EQ(program.NumParameterBlocks(), 1); - EXPECT_EQ(program.NumResidualBlocks(), 1); + scoped_ptr reduced_program( + CHECK_NOTNULL(problem + .program() + .CreateReducedProgram(&removed_parameter_blocks, + &fixed_cost, + &message))); + EXPECT_EQ(reduced_program->NumParameterBlocks(), 1); + EXPECT_EQ(reduced_program->NumResidualBlocks(), 1); } TEST(Program, RemoveFixedBlocksNumEliminateBlocks) { @@ -186,15 +195,17 @@ TEST(Program, RemoveFixedBlocksNumEliminateBlocks) { problem.AddResidualBlock(new BinaryCostFunction(), NULL, &x, &y); problem.SetParameterBlockConstant(&x); - Program program(problem.program()); vector removed_parameter_blocks; double fixed_cost = 0.0; string message; - EXPECT_TRUE(program.RemoveFixedBlocks(&removed_parameter_blocks, - &fixed_cost, - &message)); - EXPECT_EQ(program.NumParameterBlocks(), 2); - EXPECT_EQ(program.NumResidualBlocks(), 2); + scoped_ptr reduced_program( + CHECK_NOTNULL(problem + .program() + .CreateReducedProgram(&removed_parameter_blocks, + &fixed_cost, + &message))); + EXPECT_EQ(reduced_program->NumParameterBlocks(), 2); + EXPECT_EQ(reduced_program->NumResidualBlocks(), 2); } TEST(Program, RemoveFixedBlocksFixedCost) { @@ -211,27 +222,29 @@ TEST(Program, RemoveFixedBlocksFixedCost) { problem.AddResidualBlock(new BinaryCostFunction(), NULL, &x, &y); problem.SetParameterBlockConstant(&x); - Program program(problem.program()); - - double expected_fixed_cost; - ResidualBlock *expected_removed_block = program.residual_blocks()[0]; + ResidualBlock *expected_removed_block = problem.program().residual_blocks()[0]; scoped_array scratch( new double[expected_removed_block->NumScratchDoublesForEvaluate()]); + double expected_fixed_cost; expected_removed_block->Evaluate(true, &expected_fixed_cost, NULL, NULL, scratch.get()); + vector removed_parameter_blocks; double fixed_cost = 0.0; string message; - EXPECT_TRUE(program.RemoveFixedBlocks(&removed_parameter_blocks, - &fixed_cost, - &message)); + scoped_ptr reduced_program( + CHECK_NOTNULL(problem + .program() + .CreateReducedProgram(&removed_parameter_blocks, + &fixed_cost, + &message))); - EXPECT_EQ(program.NumParameterBlocks(), 2); - EXPECT_EQ(program.NumResidualBlocks(), 2); + EXPECT_EQ(reduced_program->NumParameterBlocks(), 2); + EXPECT_EQ(reduced_program->NumResidualBlocks(), 2); EXPECT_DOUBLE_EQ(fixed_cost, expected_fixed_cost); } diff --git a/internal/ceres/solver_impl.cc b/internal/ceres/solver_impl.cc index 11398b1cc..dfdfecb78 100644 --- a/internal/ceres/solver_impl.cc +++ b/internal/ceres/solver_impl.cc @@ -664,42 +664,44 @@ Program* SolverImpl::CreateReducedProgram(Solver::Options* options, string* error) { CHECK_NOTNULL(options->linear_solver_ordering.get()); Program* original_program = problem_impl->mutable_program(); - scoped_ptr transformed_program(new Program(*original_program)); + + vector removed_parameter_blocks; + scoped_ptr reduced_program( + original_program->CreateReducedProgram(&removed_parameter_blocks, + fixed_cost, + error)); + if (reduced_program.get() == NULL) { + return NULL; + } + + VLOG(2) << "Reduced problem: " + << reduced_program->NumParameterBlocks() + << " parameter blocks, " + << reduced_program->NumParameters() + << " parameters, " + << reduced_program->NumResidualBlocks() + << " residual blocks, " + << reduced_program->NumResiduals() + << " residuals."; + + if (reduced_program->NumParameterBlocks() == 0) { + LOG(WARNING) << "No varying parameter blocks to optimize; " + << "bailing early."; + return reduced_program.release(); + } ParameterBlockOrdering* linear_solver_ordering = options->linear_solver_ordering.get(); const int min_group_id = linear_solver_ordering->group_to_elements().begin()->first; - vector removed_parameter_blocks; - if (!transformed_program->RemoveFixedBlocks(&removed_parameter_blocks, - fixed_cost, - error)) { - return NULL; - } - linear_solver_ordering->Remove(removed_parameter_blocks); + ParameterBlockOrdering* inner_iteration_ordering = options->inner_iteration_ordering.get(); if (inner_iteration_ordering != NULL) { inner_iteration_ordering->Remove(removed_parameter_blocks); } - VLOG(2) << "Reduced problem: " - << transformed_program->NumParameterBlocks() - << " parameter blocks, " - << transformed_program->NumParameters() - << " parameters, " - << transformed_program->NumResidualBlocks() - << " residual blocks, " - << transformed_program->NumResiduals() - << " residuals."; - - if (transformed_program->NumParameterBlocks() == 0) { - LOG(WARNING) << "No varying parameter blocks to optimize; " - << "bailing early."; - return transformed_program.release(); - } - if (IsSchurType(options->linear_solver_type) && linear_solver_ordering->GroupSize(min_group_id) == 0) { // If the user requested the use of a Schur type solver, and @@ -730,11 +732,11 @@ Program* SolverImpl::CreateReducedProgram(Solver::Options* options, options->sparse_linear_algebra_library_type, problem_impl->parameter_map(), linear_solver_ordering, - transformed_program.get(), + reduced_program.get(), error)) { return NULL; } - return transformed_program.release(); + return reduced_program.release(); } if (options->linear_solver_type == SPARSE_NORMAL_CHOLESKY && @@ -742,16 +744,16 @@ Program* SolverImpl::CreateReducedProgram(Solver::Options* options, if (!ReorderProgramForSparseNormalCholesky( options->sparse_linear_algebra_library_type, linear_solver_ordering, - transformed_program.get(), + reduced_program.get(), error)) { return NULL; } - return transformed_program.release(); + return reduced_program.release(); } - transformed_program->SetParameterOffsetsAndIndex(); - return transformed_program.release(); + reduced_program->SetParameterOffsetsAndIndex(); + return reduced_program.release(); } LinearSolver* SolverImpl::CreateLinearSolver(Solver::Options* options,