Skip to content
This repository has been archived by the owner on Oct 26, 2018. It is now read-only.

Commit

Permalink
Solver::Options uses shared_ptr to handle ownership.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@sandwichmaker
sandwichmaker committed Apr 25, 2014
1 parent 8e09913 commit bb05be3
Show file tree
Hide file tree
Showing 10 changed files with 145 additions and 121 deletions.
83 changes: 63 additions & 20 deletions CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -395,31 +395,28 @@ ENDIF (OPENMP)
INCLUDE(CheckIncludeFileCXX)
CHECK_INCLUDE_FILE_CXX(unordered_map HAVE_STD_UNORDERED_MAP_HEADER)
IF (HAVE_STD_UNORDERED_MAP_HEADER)
# Even so we've found unordered_map header file it doesn't
# mean unordered_map and unordered_set will be declared in
# std namespace.
# Finding the unordered_map header doesn't mean that unordered_map
# is in std namespace.
#
# Namely, MSVC 2008 have unordered_map header which declares
# unordered_map class in std::tr1 namespace. In order to support
# this, we do extra check to see which exactly namespace is
# to be used.

# In particular, MSVC 2008 has unordered_map declared in std::tr1.
# In order to support this, we do an extra check to see which
# namespace should be used.
INCLUDE(CheckCXXSourceCompiles)
CHECK_CXX_SOURCE_COMPILES("#include <unordered_map>
int main() {
std::unordered_map<int, int> map;
return 0;
}"
int main() {
std::unordered_map<int, int> map;
return 0;
}"
HAVE_UNORDERED_MAP_IN_STD_NAMESPACE)
IF (HAVE_UNORDERED_MAP_IN_STD_NAMESPACE)
ADD_DEFINITIONS(-DCERES_STD_UNORDERED_MAP)
MESSAGE("-- Found unordered_map/set in std namespace.")
ELSE (HAVE_UNORDERED_MAP_IN_STD_NAMESPACE)
CHECK_CXX_SOURCE_COMPILES("#include <unordered_map>
int main() {
std::tr1::unordered_map<int, int> map;
return 0;
}"
int main() {
std::tr1::unordered_map<int, int> map;
return 0;
}"
HAVE_UNORDERED_MAP_IN_TR1_NAMESPACE)
IF (HAVE_UNORDERED_MAP_IN_TR1_NAMESPACE)
ADD_DEFINITIONS(-DCERES_STD_UNORDERED_MAP_IN_TR1_NAMESPACE)
Expand All @@ -432,17 +429,63 @@ IF (HAVE_STD_UNORDERED_MAP_HEADER)
ENDIF (HAVE_UNORDERED_MAP_IN_TR1_NAMESPACE)
ENDIF (HAVE_UNORDERED_MAP_IN_STD_NAMESPACE)
ELSE (HAVE_STD_UNORDERED_MAP_HEADER)
CHECK_INCLUDE_FILE_CXX("tr1/unordered_map" UNORDERED_MAP_IN_TR1_NAMESPACE)
IF (UNORDERED_MAP_IN_TR1_NAMESPACE)
CHECK_INCLUDE_FILE_CXX("tr1/unordered_map" HAVE_TR1_UNORDERED_MAP_HEADER)
IF (HAVE_TR1_UNORDERED_MAP_HEADER)
ADD_DEFINITIONS(-DCERES_TR1_UNORDERED_MAP)
MESSAGE("-- Found tr1/unordered_map/set in std::tr1 namespace.")
ELSE (UNORDERED_MAP_IN_TR1_NAMESPACE)
ELSE (HAVE_TR1_UNORDERED_MAP_HEADE)
MESSAGE("-- Unable to find <unordered_map> or <tr1/unordered_map>. ")
MESSAGE("-- Replacing unordered_map/set with map/set (warning: slower!)")
ADD_DEFINITIONS(-DCERES_NO_UNORDERED_MAP)
ENDIF (UNORDERED_MAP_IN_TR1_NAMESPACE)
ENDIF (HAVE_TR1_UNORDERED_MAP_HEADER)
ENDIF (HAVE_STD_UNORDERED_MAP_HEADER)

CHECK_INCLUDE_FILE_CXX(memory HAVE_STD_MEMORY_HEADER)
IF (HAVE_STD_MEMORY_HEADER)
# Finding the memory header doesn't mean that shared_ptr is in std
# namespace.
#
# In particular, MSVC 2008 has shared_ptr declared in std::tr1. In
# order to support this, we do an extra check to see which namespace
# should be used.
INCLUDE(CheckCXXSourceCompiles)
CHECK_CXX_SOURCE_COMPILES("#include <memory>
int main() {
std::shared_ptr<int> int_ptr;
return 0;
}"
HAVE_SHARED_PTR_IN_STD_NAMESPACE)

IF (HAVE_SHARED_PTR_IN_STD_NAMESPACE)
ADD_DEFINITIONS(-DCERES_STD_SHARED_PTR)
MESSAGE("-- Found shared_ptr in std namespace.")
ELSE (HAVE_SHARED_PTR_IN_STD_NAMESPACE)
CHECK_CXX_SOURCE_COMPILES("#include <memory>
int main() {
std::tr1::shared_ptr<int> int_ptr;
return 0;
}"
HAVE_SHARED_PTR_IN_TR1_NAMESPACE)
IF (HAVE_SHARED_PTR_IN_TR1_NAMESPACE)
ADD_DEFINITIONS(-DCERES_STD_SHARED_PTR_IN_TR1_NAMESPACE)
MESSAGE("-- Found shared_ptr in std::tr1 namespace.")
ELSE (HAVE_SHARED_PTR_IN_TR1_NAMESPACE)
MESSAGE(FATAL_ERROR "-- Found <memory> but cannot find either "
"std::shared_ptr or std::tr1::shared_ptr")
ADD_DEFINITIONS(-DCERES_NO_SHARED_PTR)
ENDIF (HAVE_SHARED_PTR_IN_TR1_NAMESPACE)
ENDIF (HAVE_SHARED_PTR_IN_STD_NAMESPACE)
ELSE (HAVE_STD_MEMORY_HEADER)
CHECK_INCLUDE_FILE_CXX("tr1/memory" HAVE_TR1_MEMORY_HEADER)
IF (HAVE_TR1_MEMORY_HEADER)
ADD_DEFINITIONS(-DCERES_TR1_SHARED_PTR)
MESSAGE("-- Found tr1/memory in std::tr1 namespace.")
ELSE (HAVE_TR1_MEMORY_HEADER)
MESSAGE(FATAL_ERROR "-- Unable to find <memory> or <tr1/memory>. ")
ENDIF (HAVE_TR1_MEMORY_HEADER)
ENDIF (HAVE_STD_MEMORY_HEADER)


INCLUDE_DIRECTORIES(
include
internal
Expand Down
10 changes: 5 additions & 5 deletions examples/bundle_adjuster.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,19 +151,19 @@ void SetOrdering(BALProblem* bal_problem, Solver::Options* options) {
if (options->use_inner_iterations) {
if (FLAGS_blocks_for_inner_iterations == "cameras") {
LOG(INFO) << "Camera blocks for inner iterations";
options->inner_iteration_ordering = new ParameterBlockOrdering;
options->inner_iteration_ordering.reset(new ParameterBlockOrdering);
for (int i = 0; i < num_cameras; ++i) {
options->inner_iteration_ordering->AddElementToGroup(cameras + camera_block_size * i, 0);
}
} else if (FLAGS_blocks_for_inner_iterations == "points") {
LOG(INFO) << "Point blocks for inner iterations";
options->inner_iteration_ordering = new ParameterBlockOrdering;
options->inner_iteration_ordering.reset(new ParameterBlockOrdering);
for (int i = 0; i < num_points; ++i) {
options->inner_iteration_ordering->AddElementToGroup(points + point_block_size * i, 0);
}
} else if (FLAGS_blocks_for_inner_iterations == "cameras,points") {
LOG(INFO) << "Camera followed by point blocks for inner iterations";
options->inner_iteration_ordering = new ParameterBlockOrdering;
options->inner_iteration_ordering.reset(new ParameterBlockOrdering);
for (int i = 0; i < num_cameras; ++i) {
options->inner_iteration_ordering->AddElementToGroup(cameras + camera_block_size * i, 0);
}
Expand All @@ -172,7 +172,7 @@ void SetOrdering(BALProblem* bal_problem, Solver::Options* options) {
}
} else if (FLAGS_blocks_for_inner_iterations == "points,cameras") {
LOG(INFO) << "Point followed by camera blocks for inner iterations";
options->inner_iteration_ordering = new ParameterBlockOrdering;
options->inner_iteration_ordering.reset(new ParameterBlockOrdering);
for (int i = 0; i < num_cameras; ++i) {
options->inner_iteration_ordering->AddElementToGroup(cameras + camera_block_size * i, 1);
}
Expand Down Expand Up @@ -221,7 +221,7 @@ void SetOrdering(BALProblem* bal_problem, Solver::Options* options) {
}
}

options->linear_solver_ordering = ordering;
options->linear_solver_ordering.reset(ordering);
}

void SetMinimizerOptions(Solver::Options* options) {
Expand Down
15 changes: 15 additions & 0 deletions include/ceres/internal/port.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,13 @@

#include <string>

#if defined(CERES_STD_SHARED_PTR) || defined(CERES_STD_SHARED_PTR_IN_TR1_NAMESPACE)
#include <memory>
#else
#include <tr1/memory>
#endif


namespace ceres {

// It is unfortunate that this import of the entire standard namespace is
Expand All @@ -45,6 +52,14 @@ using namespace std;
// "string" implementation in the global namespace.
using std::string;

#if defined(CERES_STD_SHARED_PTR)
using std::shared_ptr;
#endif

#if defined(CERES_STD_SHARED_PTR_IN_TR1_NAMESPACE) || defined(SHARED_PTR_IN_TR1_NAMESPACE)
using std::tr1::shared_ptr;
#endif

} // namespace ceres

#endif // CERES_PUBLIC_INTERNAL_PORT_H_
10 changes: 2 additions & 8 deletions include/ceres/solver.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,15 +107,13 @@ class Solver {


num_linear_solver_threads = 1;
linear_solver_ordering = NULL;
use_postordering = false;
min_linear_solver_iterations = 1;
max_linear_solver_iterations = 500;
eta = 1e-1;
jacobi_scaling = true;
use_inner_iterations = false;
inner_iteration_tolerance = 1e-3;
inner_iteration_ordering = NULL;
logging_type = PER_MINIMIZER_ITERATION;
minimizer_progress_to_stdout = false;
trust_region_problem_dump_directory = "/tmp";
Expand All @@ -126,7 +124,6 @@ class Solver {
update_state_every_iteration = false;
}

~Options();
// Minimizer options ----------------------------------------

// Ceres supports the two major families of optimization strategies -
Expand Down Expand Up @@ -480,10 +477,7 @@ class Solver {
// the parameter blocks into two groups, one for the points and one
// for the cameras, where the group containing the points has an id
// smaller than the group containing cameras.
//
// Once assigned, Solver::Options owns this pointer and will
// deallocate the memory when destroyed.
ParameterBlockOrdering* linear_solver_ordering;
shared_ptr<ParameterBlockOrdering> linear_solver_ordering;

// Sparse Cholesky factorization algorithms use a fill-reducing
// ordering to permute the columns of the Jacobian matrix. There
Expand Down Expand Up @@ -576,7 +570,7 @@ class Solver {
// the lower numbered groups are optimized before the higher
// number groups. Each group must be an independent set. Not
// all parameter blocks need to be present in the ordering.
ParameterBlockOrdering* inner_iteration_ordering;
shared_ptr<ParameterBlockOrdering> inner_iteration_ordering;

// Generally speaking, inner iterations make significant progress
// in the early stages of the solve and then their contribution
Expand Down
5 changes: 0 additions & 5 deletions internal/ceres/solver.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,6 @@ void StringifyOrdering(const vector<int>& ordering, string* report) {

} // namespace

Solver::Options::~Options() {
delete linear_solver_ordering;
delete inner_iteration_ordering;
}

Solver::~Solver() {}

void Solver::Solve(const Solver::Options& options,
Expand Down
48 changes: 17 additions & 31 deletions internal/ceres/solver_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -565,14 +565,12 @@ void SolverImpl::TrustRegionSolve(const Solver::Options& original_options,
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) {
Expand Down Expand Up @@ -636,17 +634,14 @@ void SolverImpl::TrustRegionSolve(const Solver::Options& original_options,
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();
Expand All @@ -657,13 +652,6 @@ void SolverImpl::TrustRegionSolve(const Solver::Options& original_options,
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,
Expand All @@ -676,7 +664,7 @@ void SolverImpl::TrustRegionSolve(const Solver::Options& original_options,
return;
}

SummarizeOrdering(options.linear_solver_ordering,
SummarizeOrdering(options.linear_solver_ordering.get(),
&(summary->linear_solver_ordering_used));
SummarizeReducedProgram(*reduced_program, summary);

Expand Down Expand Up @@ -839,8 +827,7 @@ void SolverImpl::LineSearchSolve(const Solver::Options& original_options,
// 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) {
Expand All @@ -860,15 +847,13 @@ void SolverImpl::LineSearchSolve(const Solver::Options& original_options,
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();
Expand All @@ -878,6 +863,7 @@ void SolverImpl::LineSearchSolve(const Solver::Options& original_options,
}
}


original_program->SetParameterBlockStatePtrsToUserStatePtrs();

// If the user requests gradient checking, construct a new
Expand Down Expand Up @@ -1134,16 +1120,16 @@ Program* SolverImpl::CreateReducedProgram(Solver::Options* options,
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,
Expand Down Expand Up @@ -1216,7 +1202,7 @@ Program* SolverImpl::CreateReducedProgram(Solver::Options* options,
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) {
Expand Down Expand Up @@ -1486,7 +1472,7 @@ CoordinateDescentMinimizer* SolverImpl::CreateInnerIterationMinimizer(
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
Expand All @@ -1513,7 +1499,7 @@ CoordinateDescentMinimizer* SolverImpl::CreateInnerIterationMinimizer(
return NULL;
}
}
ordering_ptr = options.inner_iteration_ordering;
ordering_ptr = options.inner_iteration_ordering.get();
}

if (!inner_iteration_minimizer->Init(program,
Expand Down
Loading

0 comments on commit bb05be3

Please sign in to comment.