Let Problem::SetParameterization be called more than once. https://github.com/ceres-solver/ceres-solver/issues/501 Change-Id: Ia94e6e62553e97fa2052db2cbebc5c472e26a406
diff --git a/docs/source/nnls_modeling.rst b/docs/source/nnls_modeling.rst index c19a4ef..2604027 100644 --- a/docs/source/nnls_modeling.rst +++ b/docs/source/nnls_modeling.rst
@@ -1762,8 +1762,8 @@ The local_parameterization is owned by the Problem by default. It is acceptable to set the same parameterization for multiple parameters; the destructor is careful to delete local - parameterizations only once. The local parameterization can only be - set once per parameter, and cannot be changed once set. + parameterizations only once. Calling `SetParameterization` with + `nullptr` will clear any previously set parameterization. .. function:: LocalParameterization* Problem::GetParameterization(const double* values) const
diff --git a/include/ceres/problem.h b/include/ceres/problem.h index a4a6c8f..ba9b903 100644 --- a/include/ceres/problem.h +++ b/include/ceres/problem.h
@@ -315,8 +315,8 @@ // The local_parameterization is owned by the Problem by default. It // is acceptable to set the same parameterization for multiple // parameters; the destructor is careful to delete local - // parameterizations only once. The local parameterization can only - // be set once per parameter, and cannot be changed once set. + // parameterizations only once. Calling SetParameterization with + // nullptr will clear any previously set parameterization. void SetParameterization(double* values, LocalParameterization* local_parameterization);
diff --git a/internal/ceres/parameter_block.h b/internal/ceres/parameter_block.h index 620c4ef..3b7ae49 100644 --- a/internal/ceres/parameter_block.h +++ b/internal/ceres/parameter_block.h
@@ -1,5 +1,5 @@ // Ceres Solver - A fast non-linear least squares minimizer -// Copyright 2015 Google Inc. All rights reserved. +// Copyright 2019 Google Inc. All rights reserved. // http://ceres-solver.org/ // // Redistribution and use in source and binary forms, with or without @@ -38,6 +38,7 @@ #include <memory> #include <string> #include <unordered_set> + #include "ceres/array_utils.h" #include "ceres/internal/eigen.h" #include "ceres/internal/port.h" @@ -166,23 +167,19 @@ : local_parameterization_->LocalSize(); } - // Set the parameterization. The parameterization can be set exactly once; - // multiple calls to set the parameterization to different values will crash. - // It is an error to pass nullptr for the parameterization. The parameter - // block does not take ownership of the parameterization. + // Set the parameterization. The parameter block does not take + // ownership of the parameterization. void SetParameterization(LocalParameterization* new_parameterization) { - CHECK(new_parameterization != nullptr) - << "nullptr parameterization invalid."; // Nothing to do if the new parameterization is the same as the // old parameterization. if (new_parameterization == local_parameterization_) { return; } - CHECK(local_parameterization_ == nullptr) - << "Can't re-set the local parameterization; it leads to " - << "ambiguous ownership. Current local parameterization is: " - << local_parameterization_; + if (new_parameterization == nullptr) { + local_parameterization_ = nullptr; + return; + } CHECK(new_parameterization->GlobalSize() == size_) << "Invalid parameterization for parameter block. The parameter block "
diff --git a/internal/ceres/parameter_block_test.cc b/internal/ceres/parameter_block_test.cc index babd354..4b65c40 100644 --- a/internal/ceres/parameter_block_test.cc +++ b/internal/ceres/parameter_block_test.cc
@@ -30,8 +30,8 @@ #include "ceres/parameter_block.h" -#include "gtest/gtest.h" #include "ceres/internal/eigen.h" +#include "gtest/gtest.h" namespace ceres { namespace internal { @@ -56,37 +56,6 @@ parameter_block.SetParameterization(&subset); } -TEST(ParameterBlock, SetLocalParameterizationDiesWhenResettingToNull) { - double x[3] = {1.0, 2.0, 3.0}; - ParameterBlock parameter_block(x, 3, -1); - std::vector<int> indices; - indices.push_back(1); - SubsetParameterization subset(3, indices); - parameter_block.SetParameterization(&subset); - EXPECT_DEATH_IF_SUPPORTED(parameter_block.SetParameterization(nullptr), "nullptr"); -} - -TEST(ParameterBlock, - SetLocalParameterizationDiesWhenResettingToDifferentParameterization) { - double x[3] = {1.0, 2.0, 3.0}; - ParameterBlock parameter_block(x, 3, -1); - std::vector<int> indices; - indices.push_back(1); - SubsetParameterization subset(3, indices); - parameter_block.SetParameterization(&subset); - SubsetParameterization subset_different(3, indices); - EXPECT_DEATH_IF_SUPPORTED( - parameter_block.SetParameterization(&subset_different), "re-set"); -} - -TEST(ParameterBlock, SetLocalParameterizationDiesOnNullParameterization) { - double x[3] = {1.0, 2.0, 3.0}; - ParameterBlock parameter_block(x, 3, -1); - std::vector<int> indices; - indices.push_back(1); - EXPECT_DEATH_IF_SUPPORTED(parameter_block.SetParameterization(nullptr), "nullptr"); -} - TEST(ParameterBlock, SetParameterizationDiesOnZeroLocalSize) { double x[3] = {1.0, 2.0, 3.0}; ParameterBlock parameter_block(x, 3, -1); @@ -100,7 +69,7 @@ } TEST(ParameterBlock, SetLocalParameterizationAndNormalOperation) { - double x[3] = { 1.0, 2.0, 3.0 }; + double x[3] = {1.0, 2.0, 3.0}; ParameterBlock parameter_block(x, 3, -1); std::vector<int> indices; indices.push_back(1); @@ -109,9 +78,7 @@ // Ensure the local parameterization jacobian result is correctly computed. ConstMatrixRef local_parameterization_jacobian( - parameter_block.LocalParameterizationJacobian(), - 3, - 2); + parameter_block.LocalParameterizationJacobian(), 3, 2); ASSERT_EQ(1.0, local_parameterization_jacobian(0, 0)); ASSERT_EQ(0.0, local_parameterization_jacobian(0, 1)); ASSERT_EQ(0.0, local_parameterization_jacobian(1, 0)); @@ -121,7 +88,7 @@ // Check that updating works as expected. double x_plus_delta[3]; - double delta[2] = { 0.5, 0.3 }; + double delta[2] = {0.5, 0.3}; parameter_block.Plus(x, delta, x_plus_delta); ASSERT_EQ(1.5, x_plus_delta[0]); ASSERT_EQ(2.0, x_plus_delta[1]); @@ -137,8 +104,7 @@ LOG(FATAL) << "Shouldn't get called."; return true; } - bool ComputeJacobian(const double* x, - double* jacobian) const final { + bool ComputeJacobian(const double* x, double* jacobian) const final { jacobian[0] = *x * 2; return true; } @@ -149,7 +115,7 @@ TEST(ParameterBlock, SetStateUpdatesLocalParameterizationJacobian) { TestParameterization test_parameterization; - double x[1] = { 1.0 }; + double x[1] = {1.0}; ParameterBlock parameter_block(x, 1, -1, &test_parameterization); EXPECT_EQ(2.0, *parameter_block.LocalParameterizationJacobian()); @@ -160,10 +126,10 @@ } TEST(ParameterBlock, PlusWithNoLocalParameterization) { - double x[2] = { 1.0, 2.0 }; + double x[2] = {1.0, 2.0}; ParameterBlock parameter_block(x, 2, -1); - double delta[2] = { 0.2, 0.3 }; + double delta[2] = {0.2, 0.3}; double x_plus_delta[2]; parameter_block.Plus(x, delta, x_plus_delta); EXPECT_EQ(1.2, x_plus_delta[0]); @@ -173,9 +139,7 @@ // Stops computing the jacobian after the first time. class BadLocalParameterization : public LocalParameterization { public: - BadLocalParameterization() - : calls_(0) { - } + BadLocalParameterization() : calls_(0) {} virtual ~BadLocalParameterization() {} bool Plus(const double* x, @@ -193,8 +157,8 @@ return true; } - int GlobalSize() const final { return 1;} - int LocalSize() const final { return 1;} + int GlobalSize() const final { return 1; } + int LocalSize() const final { return 1; } private: mutable int calls_; @@ -248,5 +212,40 @@ EXPECT_EQ(x_plus_delta[1], -1.0); } +TEST(ParameterBlock, ResetLocalParameterizationToNull) { + double x[3] = {1.0, 2.0, 3.0}; + ParameterBlock parameter_block(x, 3, -1); + std::vector<int> indices; + indices.push_back(1); + SubsetParameterization subset(3, indices); + parameter_block.SetParameterization(&subset); + EXPECT_EQ(parameter_block.local_parameterization(), &subset); + parameter_block.SetParameterization(nullptr); + EXPECT_EQ(parameter_block.local_parameterization(), nullptr); +} + +TEST(ParameterBlock, ResetLocalParameterizationToNotNull) { + double x[3] = {1.0, 2.0, 3.0}; + ParameterBlock parameter_block(x, 3, -1); + std::vector<int> indices; + indices.push_back(1); + SubsetParameterization subset(3, indices); + parameter_block.SetParameterization(&subset); + EXPECT_EQ(parameter_block.local_parameterization(), &subset); + + SubsetParameterization subset_different(3, indices); + parameter_block.SetParameterization(&subset_different); + EXPECT_EQ(parameter_block.local_parameterization(), &subset_different); +} + +TEST(ParameterBlock, SetNullLocalParameterization) { + double x[3] = {1.0, 2.0, 3.0}; + ParameterBlock parameter_block(x, 3, -1); + EXPECT_EQ(parameter_block.local_parameterization(), nullptr); + + parameter_block.SetParameterization(nullptr); + EXPECT_EQ(parameter_block.local_parameterization(), nullptr); +} + } // namespace internal } // namespace ceres
diff --git a/internal/ceres/problem_impl.cc b/internal/ceres/problem_impl.cc index 63dad12..7a36c2b 100644 --- a/internal/ceres/problem_impl.cc +++ b/internal/ceres/problem_impl.cc
@@ -1,5 +1,5 @@ // Ceres Solver - A fast non-linear least squares minimizer -// Copyright 2015 Google Inc. All rights reserved. +// Copyright 2019 Google Inc. All rights reserved. // http://ceres-solver.org/ // // Redistribution and use in source and binary forms, with or without @@ -113,7 +113,7 @@ void InitializeContext(Context* context, ContextImpl** context_impl, bool* context_impl_owned) { - if (context == NULL) { + if (context == nullptr) { *context_impl_owned = true; *context_impl = new ContextImpl; } else { @@ -126,8 +126,8 @@ ParameterBlock* ProblemImpl::InternalAddParameterBlock(double* values, int size) { - CHECK(values != NULL) << "Null pointer passed to AddParameterBlock " - << "for a parameter with size " << size; + CHECK(values != nullptr) << "Null pointer passed to AddParameterBlock " + << "for a parameter with size " << size; // Ignore the request if there is a block for the given pointer already. ParameterMap::iterator it = parameter_block_map_.find(values); @@ -216,7 +216,7 @@ LossFunction* loss_function = const_cast<LossFunction*>(residual_block->loss_function()); if (options_.loss_function_ownership == TAKE_OWNERSHIP && - loss_function != NULL) { + loss_function != nullptr) { DecrementValueOrDeleteKey(loss_function, &loss_function_ref_count_); } @@ -230,7 +230,7 @@ // without doing a full scan. void ProblemImpl::DeleteBlock(ParameterBlock* parameter_block) { if (options_.local_parameterization_ownership == TAKE_OWNERSHIP && - parameter_block->local_parameterization() != NULL) { + parameter_block->local_parameterization() != nullptr) { local_parameterizations_to_delete_.push_back( parameter_block->mutable_local_parameterization()); } @@ -361,7 +361,7 @@ } if (options_.loss_function_ownership == TAKE_OWNERSHIP && - loss_function != NULL) { + loss_function != nullptr) { ++loss_function_ref_count_[loss_function]; } @@ -375,7 +375,7 @@ void ProblemImpl::AddParameterBlock( double* values, int size, LocalParameterization* local_parameterization) { ParameterBlock* parameter_block = InternalAddParameterBlock(values, size); - if (local_parameterization != NULL) { + if (local_parameterization != nullptr) { parameter_block->SetParameterization(local_parameterization); } } @@ -519,6 +519,15 @@ << "you can set its local parameterization."; } + // If the parameter block already has a local parameterization and + // we are to take ownership of local parameterizations, then add it + // to local_parameterizations_to_delete_ for eventual deletion. + if (parameter_block->local_parameterization_ && + options_.local_parameterization_ownership == TAKE_OWNERSHIP) { + local_parameterizations_to_delete_.push_back( + parameter_block->local_parameterization_); + } + parameter_block->SetParameterization(local_parameterization); }
diff --git a/internal/ceres/problem_impl.h b/internal/ceres/problem_impl.h index f366b7d..8bbe723 100644 --- a/internal/ceres/problem_impl.h +++ b/internal/ceres/problem_impl.h
@@ -1,5 +1,5 @@ // Ceres Solver - A fast non-linear least squares minimizer -// Copyright 2015 Google Inc. All rights reserved. +// Copyright 2019 Google Inc. All rights reserved. // http://ceres-solver.org/ // // Redistribution and use in source and binary forms, with or without
diff --git a/internal/ceres/problem_test.cc b/internal/ceres/problem_test.cc index b779524..12e8dc2 100644 --- a/internal/ceres/problem_test.cc +++ b/internal/ceres/problem_test.cc
@@ -2096,5 +2096,32 @@ std::numeric_limits<double>::max()); } +TEST(Problem, SetParameterizationTwice) { + Problem problem; + double x[] = {1.0, 2.0, 3.0}; + problem.AddParameterBlock(x, 3); + problem.SetParameterization(x, new SubsetParameterization(3, {1})); + EXPECT_EQ(problem.GetParameterization(x)->GlobalSize(), 3); + EXPECT_EQ(problem.GetParameterization(x)->LocalSize(), 2); + + problem.SetParameterization(x, new SubsetParameterization(3, {0, 1})); + EXPECT_EQ(problem.GetParameterization(x)->GlobalSize(), 3); + EXPECT_EQ(problem.GetParameterization(x)->LocalSize(), 1); +} + +TEST(Problem, SetParameterizationAndThenClearItWithNull) { + Problem problem; + double x[] = {1.0, 2.0, 3.0}; + problem.AddParameterBlock(x, 3); + problem.SetParameterization(x, new SubsetParameterization(3, {1})); + EXPECT_EQ(problem.GetParameterization(x)->GlobalSize(), 3); + EXPECT_EQ(problem.GetParameterization(x)->LocalSize(), 2); + + problem.SetParameterization(x, nullptr); + EXPECT_EQ(problem.GetParameterization(x), nullptr); + EXPECT_EQ(problem.ParameterBlockLocalSize(x), 3); + EXPECT_EQ(problem.ParameterBlockSize(x), 3); +} + } // namespace internal } // namespace ceres