Solver::Options uses shared_ptr to handle ownership.
Solver::Options::linear_solver_ordering and
Solver::Options::inner_iteration_ordering
were bare pointers even though Solver::Options took ownership of these
objects.
This lead to buggy user code and the inability to copy Solver::Options
objects around.
With this change, these naked pointers have been replaced by a
shared_ptr object which will managed the lifetime of these objects. This
also leads to simplification of the lifetime handling of these objects
inside the solver.
The Android.mk and Application.mk files have also been updated
to use a newer NDK revision which ships with LLVM's libc++.
Change-Id: I25161fb3ddf737be0b3e5dfd8e7a0039b22548cd
diff --git a/internal/ceres/solver.cc b/internal/ceres/solver.cc
index 36c58b3..53a9b4b 100644
--- a/internal/ceres/solver.cc
+++ b/internal/ceres/solver.cc
@@ -56,11 +56,6 @@
} // namespace
-Solver::Options::~Options() {
- delete linear_solver_ordering;
- delete inner_iteration_ordering;
-}
-
Solver::~Solver() {}
void Solver::Solve(const Solver::Options& options,
diff --git a/internal/ceres/solver_impl.cc b/internal/ceres/solver_impl.cc
index 9c5a2b0..e85ffbf 100644
--- a/internal/ceres/solver_impl.cc
+++ b/internal/ceres/solver_impl.cc
@@ -565,14 +565,12 @@
summary->minimizer_type = TRUST_REGION;
SummarizeGivenProgram(*original_program, summary);
- SummarizeOrdering(original_options.linear_solver_ordering,
+ SummarizeOrdering(original_options.linear_solver_ordering.get(),
&(summary->linear_solver_ordering_given));
- SummarizeOrdering(original_options.inner_iteration_ordering,
+ SummarizeOrdering(original_options.inner_iteration_ordering.get(),
&(summary->inner_iteration_ordering_given));
Solver::Options options(original_options);
- options.linear_solver_ordering = NULL;
- options.inner_iteration_ordering = NULL;
#ifndef CERES_USE_OPENMP
if (options.num_threads > 1) {
@@ -636,17 +634,14 @@
problem_impl = gradient_checking_problem_impl.get();
}
- if (original_options.linear_solver_ordering != NULL) {
- if (!IsOrderingValid(original_options, problem_impl, &summary->message)) {
+ if (options.linear_solver_ordering.get() != NULL) {
+ if (!IsOrderingValid(options, problem_impl, &summary->message)) {
LOG(ERROR) << summary->message;
return;
}
event_logger.AddEvent("CheckOrdering");
- options.linear_solver_ordering =
- new ParameterBlockOrdering(*original_options.linear_solver_ordering);
- event_logger.AddEvent("CopyOrdering");
} else {
- options.linear_solver_ordering = new ParameterBlockOrdering;
+ options.linear_solver_ordering.reset(new ParameterBlockOrdering);
const ProblemImpl::ParameterMap& parameter_map =
problem_impl->parameter_map();
for (ProblemImpl::ParameterMap::const_iterator it = parameter_map.begin();
@@ -657,13 +652,6 @@
event_logger.AddEvent("ConstructOrdering");
}
- if (original_options.inner_iteration_ordering != NULL) {
- // Make a copy, as the options struct takes ownership of the
- // ordering objects.
- options.inner_iteration_ordering =
- new ParameterBlockOrdering(*original_options.inner_iteration_ordering);
- }
-
// Create the three objects needed to minimize: the transformed program, the
// evaluator, and the linear solver.
scoped_ptr<Program> reduced_program(CreateReducedProgram(&options,
@@ -676,7 +664,7 @@
return;
}
- SummarizeOrdering(options.linear_solver_ordering,
+ SummarizeOrdering(options.linear_solver_ordering.get(),
&(summary->linear_solver_ordering_used));
SummarizeReducedProgram(*reduced_program, summary);
@@ -839,8 +827,7 @@
// refactored to deal with the various bits of cleanups related to
// line search.
options.linear_solver_type = CGNR;
- options.linear_solver_ordering = NULL;
- options.inner_iteration_ordering = NULL;
+
#ifndef CERES_USE_OPENMP
if (options.num_threads > 1) {
@@ -860,15 +847,13 @@
return;
}
- if (original_options.linear_solver_ordering != NULL) {
- if (!IsOrderingValid(original_options, problem_impl, &summary->message)) {
+ if (options.linear_solver_ordering.get() != NULL) {
+ if (!IsOrderingValid(options, problem_impl, &summary->message)) {
LOG(ERROR) << summary->message;
return;
}
- options.linear_solver_ordering =
- new ParameterBlockOrdering(*original_options.linear_solver_ordering);
} else {
- options.linear_solver_ordering = new ParameterBlockOrdering;
+ options.linear_solver_ordering.reset(new ParameterBlockOrdering);
const ProblemImpl::ParameterMap& parameter_map =
problem_impl->parameter_map();
for (ProblemImpl::ParameterMap::const_iterator it = parameter_map.begin();
@@ -878,6 +863,7 @@
}
}
+
original_program->SetParameterBlockStatePtrsToUserStatePtrs();
// If the user requests gradient checking, construct a new
@@ -1134,16 +1120,16 @@
ProblemImpl* problem_impl,
double* fixed_cost,
string* error) {
- CHECK_NOTNULL(options->linear_solver_ordering);
+ CHECK_NOTNULL(options->linear_solver_ordering.get());
Program* original_program = problem_impl->mutable_program();
scoped_ptr<Program> transformed_program(new Program(*original_program));
ParameterBlockOrdering* linear_solver_ordering =
- options->linear_solver_ordering;
+ options->linear_solver_ordering.get();
const int min_group_id =
linear_solver_ordering->group_to_elements().begin()->first;
ParameterBlockOrdering* inner_iteration_ordering =
- options->inner_iteration_ordering;
+ options->inner_iteration_ordering.get();
if (!RemoveFixedBlocksFromProgram(transformed_program.get(),
linear_solver_ordering,
inner_iteration_ordering,
@@ -1216,7 +1202,7 @@
LinearSolver* SolverImpl::CreateLinearSolver(Solver::Options* options,
string* error) {
CHECK_NOTNULL(options);
- CHECK_NOTNULL(options->linear_solver_ordering);
+ CHECK_NOTNULL(options->linear_solver_ordering.get());
CHECK_NOTNULL(error);
if (options->trust_region_strategy_type == DOGLEG) {
@@ -1486,7 +1472,7 @@
scoped_ptr<ParameterBlockOrdering> inner_iteration_ordering;
ParameterBlockOrdering* ordering_ptr = NULL;
- if (options.inner_iteration_ordering == NULL) {
+ if (options.inner_iteration_ordering.get() == NULL) {
// Find a recursive decomposition of the Hessian matrix as a set
// of independent sets of decreasing size and invert it. This
// seems to work better in practice, i.e., Cameras before
@@ -1513,7 +1499,7 @@
return NULL;
}
}
- ordering_ptr = options.inner_iteration_ordering;
+ ordering_ptr = options.inner_iteration_ordering.get();
}
if (!inner_iteration_minimizer->Init(program,
diff --git a/internal/ceres/solver_impl_test.cc b/internal/ceres/solver_impl_test.cc
index 671edb0..c2f8c22 100644
--- a/internal/ceres/solver_impl_test.cc
+++ b/internal/ceres/solver_impl_test.cc
@@ -326,7 +326,7 @@
Solver::Options options;
options.linear_solver_type = DENSE_SCHUR;
- options.linear_solver_ordering = linear_solver_ordering;
+ options.linear_solver_ordering.reset(linear_solver_ordering);
const vector<ResidualBlock*>& residual_blocks =
problem.program().residual_blocks();
@@ -388,7 +388,7 @@
Solver::Options options;
options.linear_solver_type = DENSE_SCHUR;
- options.linear_solver_ordering = linear_solver_ordering;
+ options.linear_solver_ordering.reset(linear_solver_ordering);
// Create the reduced program. This should remove the fixed block "z",
// marking the index to -1 at the same time. x and y also get indices.
@@ -457,7 +457,7 @@
Solver::Options options;
options.linear_solver_type = DENSE_SCHUR;
- options.linear_solver_ordering = linear_solver_ordering;
+ options.linear_solver_ordering.reset(linear_solver_ordering);
string message;
scoped_ptr<Program> reduced_program(
@@ -541,7 +541,7 @@
Solver::Options options;
options.linear_solver_type = SPARSE_NORMAL_CHOLESKY;
// CreateLinearSolver assumes a non-empty ordering.
- options.linear_solver_ordering = new ParameterBlockOrdering;
+ options.linear_solver_ordering.reset(new ParameterBlockOrdering);
string message;
EXPECT_FALSE(SolverImpl::CreateLinearSolver(&options, &message));
}
@@ -552,7 +552,7 @@
options.linear_solver_type = DENSE_QR;
options.max_linear_solver_iterations = -1;
// CreateLinearSolver assumes a non-empty ordering.
- options.linear_solver_ordering = new ParameterBlockOrdering;
+ options.linear_solver_ordering.reset(new ParameterBlockOrdering);
string message;
EXPECT_EQ(SolverImpl::CreateLinearSolver(&options, &message),
static_cast<LinearSolver*>(NULL));
@@ -563,7 +563,7 @@
options.linear_solver_type = DENSE_QR;
options.min_linear_solver_iterations = -1;
// CreateLinearSolver assumes a non-empty ordering.
- options.linear_solver_ordering = new ParameterBlockOrdering;
+ options.linear_solver_ordering.reset(new ParameterBlockOrdering);
string message;
EXPECT_EQ(SolverImpl::CreateLinearSolver(&options, &message),
static_cast<LinearSolver*>(NULL));
@@ -574,7 +574,7 @@
options.linear_solver_type = DENSE_QR;
options.min_linear_solver_iterations = 10;
options.max_linear_solver_iterations = 5;
- options.linear_solver_ordering = new ParameterBlockOrdering;
+ options.linear_solver_ordering.reset(new ParameterBlockOrdering);
string message;
EXPECT_EQ(SolverImpl::CreateLinearSolver(&options, &message),
static_cast<LinearSolver*>(NULL));
@@ -586,7 +586,7 @@
options.num_linear_solver_threads = 2;
// The Schur type solvers can only be created with the Ordering
// contains at least one elimination group.
- options.linear_solver_ordering = new ParameterBlockOrdering;
+ options.linear_solver_ordering.reset(new ParameterBlockOrdering);
double x;
double y;
options.linear_solver_ordering->AddElementToGroup(&x, 0);
@@ -604,7 +604,7 @@
Solver::Options options;
options.trust_region_strategy_type = DOGLEG;
// CreateLinearSolver assumes a non-empty ordering.
- options.linear_solver_ordering = new ParameterBlockOrdering;
+ options.linear_solver_ordering.reset(new ParameterBlockOrdering);
string message;
options.linear_solver_type = ITERATIVE_SCHUR;
EXPECT_EQ(SolverImpl::CreateLinearSolver(&options, &message),
@@ -620,7 +620,7 @@
scoped_ptr<LinearSolver> solver;
options.linear_solver_type = DENSE_QR;
// CreateLinearSolver assumes a non-empty ordering.
- options.linear_solver_ordering = new ParameterBlockOrdering;
+ options.linear_solver_ordering.reset(new ParameterBlockOrdering);
string message;
solver.reset(SolverImpl::CreateLinearSolver(&options, &message));
EXPECT_EQ(options.linear_solver_type, DENSE_QR);
diff --git a/internal/ceres/system_test.cc b/internal/ceres/system_test.cc
index eb2e24c..f4b9ff3 100644
--- a/internal/ceres/system_test.cc
+++ b/internal/ceres/system_test.cc
@@ -140,8 +140,7 @@
options.num_linear_solver_threads = config.num_threads;
if (config.use_automatic_ordering) {
- delete options.linear_solver_ordering;
- options.linear_solver_ordering = NULL;
+ options.linear_solver_ordering.reset();
}
LOG(INFO) << "Running solver configuration: "
@@ -398,7 +397,7 @@
problem_.AddResidualBlock(cost_function, NULL, camera, point);
}
- options_.linear_solver_ordering = new ParameterBlockOrdering;
+ options_.linear_solver_ordering.reset(new ParameterBlockOrdering);
// The points come before the cameras.
for (int i = 0; i < num_points_; ++i) {
