From 8f1b6123ada7c4514c7e4b3489e2124d43246b83 Mon Sep 17 00:00:00 2001 From: Sameer Agarwal Date: Wed, 14 Aug 2024 11:15:23 -0700 Subject: [PATCH] GradientProblem & related classes use std::unique_ptr Previously these classes in analogy with ceres::Problem's interface had interfaces to allow bare pointers as well as unique_ptrs. This CL changes the API to always use unique_ptr, this is less error prone and makes the default ownership semantics clearer. Change-Id: I7577a90761f341c7e009c248c820f0fec2e6f32d --- docs/source/gradient_solver.rst | 6 +- docs/source/gradient_tutorial.rst | 14 ++-- examples/rosenbrock.cc | 6 +- examples/rosenbrock_analytic_diff.cc | 2 +- examples/rosenbrock_numeric_diff.cc | 10 +-- include/ceres/autodiff_first_order_function.h | 13 ++-- include/ceres/gradient_problem.h | 10 ++- .../ceres/numeric_diff_first_order_function.h | 67 +++++++------------ .../autodiff_first_order_function_test.cc | 6 +- internal/ceres/gradient_problem.cc | 23 +++---- .../ceres/gradient_problem_solver_test.cc | 4 +- internal/ceres/gradient_problem_test.cc | 15 +++-- internal/ceres/line_search_minimizer_test.cc | 3 +- .../numeric_diff_first_order_function_test.cc | 5 +- 14 files changed, 83 insertions(+), 101 deletions(-) diff --git a/docs/source/gradient_solver.rst b/docs/source/gradient_solver.rst index 4e3fc7171..abde99cbc 100644 --- a/docs/source/gradient_solver.rst +++ b/docs/source/gradient_solver.rst @@ -54,9 +54,9 @@ Modeling class GradientProblem { public: - explicit GradientProblem(FirstOrderFunction* function); - GradientProblem(FirstOrderFunction* function, - Manifold* manifold); + explicit GradientProblem(std::unique_ptr function); + GradientProblem(std::unique_ptr manifold); int NumParameters() const; int NumTangentParameters() const; bool Evaluate(const double* parameters, double* cost, double* gradient) const; diff --git a/docs/source/gradient_tutorial.rst b/docs/source/gradient_tutorial.rst index 2af44e114..6f7508bfc 100644 --- a/docs/source/gradient_tutorial.rst +++ b/docs/source/gradient_tutorial.rst @@ -45,9 +45,10 @@ in Ceres. return true; } - static ceres::FirstOrderFunction* Create() { + static std::unique_ptr Create() { constexpr int kNumParameters = 2; - return new ceres::AutoDiffFirstOrderFunction(); + return std::make_unique< + ceres::AutoDiffFirstOrderFunction>(); } }; @@ -156,11 +157,12 @@ follows [#f2]_. return true; } - static ceres::FirstOrderFunction* Create() { + static std::unique_ptr Create() { constexpr int kNumParameters = 2; - return new ceres::NumericDiffFirstOrderFunction(); + return std::make_unique< + ceres::NumericDiffFirstOrderFunction>(); } }; diff --git a/examples/rosenbrock.cc b/examples/rosenbrock.cc index df609a0ba..d73932581 100644 --- a/examples/rosenbrock.cc +++ b/examples/rosenbrock.cc @@ -45,10 +45,10 @@ struct Rosenbrock { return true; } - static ceres::FirstOrderFunction* Create() { + static std::unique_ptr Create() { constexpr int kNumParameters = 2; - return new ceres::AutoDiffFirstOrderFunction( - new Rosenbrock); + return std::make_unique< + ceres::AutoDiffFirstOrderFunction>(); } }; diff --git a/examples/rosenbrock_analytic_diff.cc b/examples/rosenbrock_analytic_diff.cc index fb1856e40..50e1de5cc 100644 --- a/examples/rosenbrock_analytic_diff.cc +++ b/examples/rosenbrock_analytic_diff.cc @@ -68,7 +68,7 @@ int main(int argc, char** argv) { options.minimizer_progress_to_stdout = true; ceres::GradientProblemSolver::Summary summary; - ceres::GradientProblem problem(new Rosenbrock()); + ceres::GradientProblem problem(std::make_unique()); ceres::Solve(options, problem, parameters, &summary); std::cout << summary.FullReport() << "\n"; diff --git a/examples/rosenbrock_numeric_diff.cc b/examples/rosenbrock_numeric_diff.cc index 411566983..23be7f43e 100644 --- a/examples/rosenbrock_numeric_diff.cc +++ b/examples/rosenbrock_numeric_diff.cc @@ -47,12 +47,12 @@ struct Rosenbrock { return true; } - static ceres::FirstOrderFunction* Create() { + static std::unique_ptr Create() { constexpr int kNumParameters = 2; - return new ceres::NumericDiffFirstOrderFunction( - new Rosenbrock); + return std::make_unique< + ceres::NumericDiffFirstOrderFunction>(); } }; diff --git a/include/ceres/autodiff_first_order_function.h b/include/ceres/autodiff_first_order_function.h index 2dca8f3e3..0e7def381 100644 --- a/include/ceres/autodiff_first_order_function.h +++ b/include/ceres/autodiff_first_order_function.h @@ -91,7 +91,7 @@ namespace ceres { // // FirstOrderFunction* function = // new AutoDiffFirstOrderFunction( -// new QuadraticCostFunctor(1.0))); +// std::make_unique(1.0))); // // In the instantiation above, the template parameters following // "QuadraticCostFunctor", "4", describe the functor as computing a @@ -105,10 +105,13 @@ namespace ceres { template class AutoDiffFirstOrderFunction final : public FirstOrderFunction { public: - // Takes ownership of functor. - explicit AutoDiffFirstOrderFunction(FirstOrderFunctor* functor) - : AutoDiffFirstOrderFunction{ - std::unique_ptr{functor}} {} + AutoDiffFirstOrderFunction(const AutoDiffFirstOrderFunction&) = delete; + AutoDiffFirstOrderFunction& operator=(const AutoDiffFirstOrderFunction&) = + delete; + AutoDiffFirstOrderFunction(AutoDiffFirstOrderFunction&& other) noexcept = + default; + AutoDiffFirstOrderFunction& operator=( + AutoDiffFirstOrderFunction&& other) noexcept = default; explicit AutoDiffFirstOrderFunction( std::unique_ptr functor) diff --git a/include/ceres/gradient_problem.h b/include/ceres/gradient_problem.h index 96d6493d5..3d8567cb9 100644 --- a/include/ceres/gradient_problem.h +++ b/include/ceres/gradient_problem.h @@ -88,14 +88,12 @@ class FirstOrderFunction; // virtual int NumParameters() const { return 2; }; // }; // -// ceres::GradientProblem problem(new Rosenbrock()); +// ceres::GradientProblem problem(std::make_unique()); class CERES_EXPORT GradientProblem { public: - // Takes ownership of the function. - explicit GradientProblem(FirstOrderFunction* function); - - // Takes ownership of the function and the manifold. - GradientProblem(FirstOrderFunction* function, Manifold* manifold); + explicit GradientProblem(std::unique_ptr function); + GradientProblem(std::unique_ptr function, + std::unique_ptr manifold); int NumParameters() const; diff --git a/include/ceres/numeric_diff_first_order_function.h b/include/ceres/numeric_diff_first_order_function.h index a9a77986c..fbcb9a347 100644 --- a/include/ceres/numeric_diff_first_order_function.h +++ b/include/ceres/numeric_diff_first_order_function.h @@ -90,12 +90,13 @@ namespace ceres { // first order function with central differences used for computing the // derivative can be constructed as follows. // -// FirstOrderFunction* function -// = new NumericDiffFirstOrderFunction( -// new QuadraticCostFunctor(1.0)); ^ ^ ^ -// | | | -// Finite Differencing Scheme -+ | | -// Dimension of xy ------------------------+ +// std::unique_ptr function +// = std::make_unique< +// NumericDiffFirstOrderFunction>( +// std::make_unique(1.0)); ^ ^ +// | | +// Finite Differencing Scheme -----+ | +// Dimension of xy ----------------------+ // // // In the instantiation above, the template parameters following @@ -106,9 +107,10 @@ namespace ceres { // If the size of the parameter vector is not known at compile time, then an // alternate construction syntax can be used: // -// FirstOrderFunction* function -// = new NumericDiffFirstOrderFunction( -// new QuadraticCostFunctor(1.0), 4); +// std::unique_ptr function +// = std::make_unique>( +// std::make_unique(1.0), 4); // // Note that instead of passing 4 as a template argument, it is now passed as // the second argument to the constructor. @@ -117,15 +119,6 @@ template class NumericDiffFirstOrderFunction final : public FirstOrderFunction { public: - template >* = nullptr> - explicit NumericDiffFirstOrderFunction(Args&&... args) - : NumericDiffFirstOrderFunction{std::make_unique( - std::forward(args)...)} {} - NumericDiffFirstOrderFunction(const NumericDiffFirstOrderFunction&) = delete; NumericDiffFirstOrderFunction& operator=( const NumericDiffFirstOrderFunction&) = delete; @@ -134,37 +127,23 @@ class NumericDiffFirstOrderFunction final : public FirstOrderFunction { NumericDiffFirstOrderFunction& operator=( NumericDiffFirstOrderFunction&& other) noexcept = default; - // Constructor for the case where the parameter size is known at compile time. - explicit NumericDiffFirstOrderFunction( - FirstOrderFunctor* functor, - Ownership ownership = TAKE_OWNERSHIP, - const NumericDiffOptions& options = NumericDiffOptions()) - : NumericDiffFirstOrderFunction{ - std::unique_ptr{functor}, - kNumParameters, - ownership, - options, - FIXED_INIT} {} - // Constructor for the case where the parameter size is known at compile time. explicit NumericDiffFirstOrderFunction( std::unique_ptr functor, const NumericDiffOptions& options = NumericDiffOptions()) + : NumericDiffFirstOrderFunction{std::move(functor), + kNumParameters, + TAKE_OWNERSHIP, + options, + FIXED_INIT} {} + template >* = nullptr> + explicit NumericDiffFirstOrderFunction(Args&&... args) : NumericDiffFirstOrderFunction{ - std::move(functor), kNumParameters, TAKE_OWNERSHIP, FIXED_INIT} {} - - // Constructor for the case where the parameter size is specified at run time. - explicit NumericDiffFirstOrderFunction( - FirstOrderFunctor* functor, - int num_parameters, - Ownership ownership = TAKE_OWNERSHIP, - const NumericDiffOptions& options = NumericDiffOptions()) - : NumericDiffFirstOrderFunction{ - std::unique_ptr{functor}, - num_parameters, - ownership, - options, - DYNAMIC_INIT} {} + std::make_unique(std::forward(args)...)} {} // Constructor for the case where the parameter size is specified at run time. explicit NumericDiffFirstOrderFunction( diff --git a/internal/ceres/autodiff_first_order_function_test.cc b/internal/ceres/autodiff_first_order_function_test.cc index e663f136c..5f1eb0b10 100644 --- a/internal/ceres/autodiff_first_order_function_test.cc +++ b/internal/ceres/autodiff_first_order_function_test.cc @@ -53,9 +53,9 @@ class QuadraticCostFunctor { }; TEST(AutoDiffFirstOrderFunction, BilinearDifferentiationTest) { - std::unique_ptr function( - new AutoDiffFirstOrderFunction( - new QuadraticCostFunctor(1.0))); + std::unique_ptr function = + std::make_unique>( + 1.0); double parameters[4] = {1.0, 2.0, 3.0, 4.0}; double gradient[4]; diff --git a/internal/ceres/gradient_problem.cc b/internal/ceres/gradient_problem.cc index 486c99a41..71434bafa 100644 --- a/internal/ceres/gradient_problem.cc +++ b/internal/ceres/gradient_problem.cc @@ -36,24 +36,21 @@ namespace ceres { -GradientProblem::GradientProblem(FirstOrderFunction* function) - : function_(function), +GradientProblem::GradientProblem(std::unique_ptr function) + : function_(std::move(function)), manifold_(std::make_unique>( function_->NumParameters())), scratch_(new double[function_->NumParameters()]) { - CHECK(function != nullptr); + CHECK(function_ != nullptr); } -GradientProblem::GradientProblem(FirstOrderFunction* function, - Manifold* manifold) - : function_(function), scratch_(new double[function_->NumParameters()]) { - CHECK(function != nullptr); - if (manifold != nullptr) { - manifold_.reset(manifold); - } else { - manifold_ = std::make_unique>( - function_->NumParameters()); - } +GradientProblem::GradientProblem(std::unique_ptr function, + std::unique_ptr manifold) + : function_(std::move(function)), + manifold_(std::move(manifold)), + scratch_(new double[function_->NumParameters()]) { + CHECK(function_ != nullptr); + CHECK(manifold_ != nullptr); CHECK_EQ(function_->NumParameters(), manifold_->AmbientSize()); } diff --git a/internal/ceres/gradient_problem_solver_test.cc b/internal/ceres/gradient_problem_solver_test.cc index f8eabf610..52884dc68 100644 --- a/internal/ceres/gradient_problem_solver_test.cc +++ b/internal/ceres/gradient_problem_solver_test.cc @@ -61,7 +61,7 @@ TEST(GradientProblemSolver, SolvesRosenbrockWithDefaultOptions) { ceres::GradientProblemSolver::Options options; ceres::GradientProblemSolver::Summary summary; - ceres::GradientProblem problem(new Rosenbrock()); + ceres::GradientProblem problem(std::make_unique()); ceres::Solve(options, problem, parameters, &summary); EXPECT_EQ(CONVERGENCE, summary.termination_type); @@ -99,7 +99,7 @@ TEST(Solver, UpdateStateEveryIterationOption) { double x = 50.0; const double original_x = x; - ceres::GradientProblem problem(new QuadraticFunction); + ceres::GradientProblem problem(std::make_unique()); ceres::GradientProblemSolver::Options options; RememberingCallback callback(&x); options.callbacks.push_back(&callback); diff --git a/internal/ceres/gradient_problem_test.cc b/internal/ceres/gradient_problem_test.cc index 52757a3df..bebc3a7a5 100644 --- a/internal/ceres/gradient_problem_test.cc +++ b/internal/ceres/gradient_problem_test.cc @@ -64,13 +64,16 @@ class QuadraticTestFunction : public ceres::FirstOrderFunction { TEST(GradientProblem, TakesOwnershipOfFirstOrderFunction) { bool is_destructed = false; - { ceres::GradientProblem problem(new QuadraticTestFunction(&is_destructed)); } + { + ceres::GradientProblem problem( + std::make_unique(&is_destructed)); + } EXPECT_TRUE(is_destructed); } TEST(GradientProblem, EvaluationWithManifoldAndNoGradient) { - ceres::GradientProblem problem(new QuadraticTestFunction(), - new EuclideanManifold<1>); + ceres::GradientProblem problem(std::make_unique(), + std::make_unique>()); double x = 7.0; double cost = 0; problem.Evaluate(&x, &cost, nullptr); @@ -78,7 +81,7 @@ TEST(GradientProblem, EvaluationWithManifoldAndNoGradient) { } TEST(GradientProblem, EvaluationWithoutManifoldAndWithGradient) { - ceres::GradientProblem problem(new QuadraticTestFunction()); + ceres::GradientProblem problem(std::make_unique()); double x = 7.0; double cost = 0; double gradient = 0; @@ -87,8 +90,8 @@ TEST(GradientProblem, EvaluationWithoutManifoldAndWithGradient) { } TEST(GradientProblem, EvaluationWithManifoldAndWithGradient) { - ceres::GradientProblem problem(new QuadraticTestFunction(), - new EuclideanManifold<1>); + ceres::GradientProblem problem(std::make_unique(), + std::make_unique>()); double x = 7.0; double cost = 0; double gradient = 0; diff --git a/internal/ceres/line_search_minimizer_test.cc b/internal/ceres/line_search_minimizer_test.cc index 576d90e55..5225f7b10 100644 --- a/internal/ceres/line_search_minimizer_test.cc +++ b/internal/ceres/line_search_minimizer_test.cc @@ -52,7 +52,8 @@ class QuadraticFirstOrderFunction : public ceres::FirstOrderFunction { TEST(LineSearchMinimizerTest, FinalCostIsZero) { double parameters[1] = {2.0}; - ceres::GradientProblem problem(new QuadraticFirstOrderFunction); + ceres::GradientProblem problem( + std::make_unique()); ceres::GradientProblemSolver::Options options; ceres::GradientProblemSolver::Summary summary; ceres::Solve(options, problem, parameters, &summary); diff --git a/internal/ceres/numeric_diff_first_order_function_test.cc b/internal/ceres/numeric_diff_first_order_function_test.cc index ff57e2d96..c29462758 100644 --- a/internal/ceres/numeric_diff_first_order_function_test.cc +++ b/internal/ceres/numeric_diff_first_order_function_test.cc @@ -52,8 +52,7 @@ class QuadraticCostFunctor { TEST(NumericDiffFirstOrderFunction, BilinearDifferentiationTestStatic) { auto function = std::make_unique< - NumericDiffFirstOrderFunction>( - new QuadraticCostFunctor(1.0)); + NumericDiffFirstOrderFunction>(1.0); double parameters[4] = {1.0, 2.0, 3.0, 4.0}; double gradient[4]; @@ -77,7 +76,7 @@ TEST(NumericDiffFirstOrderFunction, BilinearDifferentiationTestStatic) { TEST(NumericDiffFirstOrderFunction, BilinearDifferentiationTestDynamic) { auto function = std::make_unique< NumericDiffFirstOrderFunction>( - new QuadraticCostFunctor(1.0), 4); + std::make_unique(1.0), 4); double parameters[4] = {1.0, 2.0, 3.0, 4.0}; double gradient[4];