Fix user iteration callbacks.

User callbacks got broken at some point due to the extra
layer of copying from Solver::Options to Minimizer::Options.
This copies the user callbacks when initializing
Minimizer::Options from Solver::Options, and adds a test to
this effect.

This also fixes a bug where the state updating callback was
not called before the user callbacks. This also adds a test
to solver_impl_test to ensure the state updating callbacks
work as expected.

Thanks to Luis Alberto Zarrabeitia for the report.

Issue: 46
Change-Id: I2b36415c89dafaa5c84ecaa727a325df122e1092

internal/ceres/CMakeLists.txt

@@ -182,6 +182,7 @@ IF (${BUILD_TESTING})
   CERES_TEST(levenberg_marquardt_strategy)
   CERES_TEST(local_parameterization)
   CERES_TEST(loss_function)
+  CERES_TEST(minimizer)
   CERES_TEST(normal_prior)
   CERES_TEST(numeric_diff_cost_function)
   CERES_TEST(parameter_block)

internal/ceres/minimizer.h

@@ -78,6 +78,7 @@ class Minimizer {
       evaluator = NULL;
       trust_region_strategy = NULL;
       jacobian = NULL;
+      callbacks = options.callbacks;
     }
 
     int max_num_iterations;

internal/ceres/minimizer_test.cc

@@ -0,0 +1,63 @@
+// Ceres Solver - A fast non-linear least squares minimizer
+// Copyright 2012 Google Inc. All rights reserved.
+// http://code.google.com/p/ceres-solver/
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are met:
+//
+// * Redistributions of source code must retain the above copyright notice,
+//   this list of conditions and the following disclaimer.
+// * Redistributions in binary form must reproduce the above copyright notice,
+//   this list of conditions and the following disclaimer in the documentation
+//   and/or other materials provided with the distribution.
+// * Neither the name of Google Inc. nor the names of its contributors may be
+//   used to endorse or promote products derived from this software without
+//   specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+// AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+// IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+// ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
+// LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+// CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+// SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+// INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+// CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+// ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+// POSSIBILITY OF SUCH DAMAGE.
+//
+// Author: keir@google.com (Keir Mierle)
+
+#include "gtest/gtest.h"
+#include "ceres/iteration_callback.h"
+#include "ceres/minimizer.h"
+#include "ceres/solver.h"
+
+namespace ceres {
+namespace internal {
+
+class FakeIterationCallback : public IterationCallback {
+ public:
+  virtual ~FakeIterationCallback() {}
+  virtual CallbackReturnType operator()(const IterationSummary& summary) {
+    return SOLVER_CONTINUE;
+  }
+};
+
+TEST(MinimizerTest, InitializationCopiesCallbacks) {
+  FakeIterationCallback callback0;
+  FakeIterationCallback callback1;
+
+  Solver::Options solver_options;
+  solver_options.callbacks.push_back(&callback0);
+  solver_options.callbacks.push_back(&callback1);
+
+  Minimizer::Options minimizer_options(solver_options);
+  ASSERT_EQ(2, minimizer_options.callbacks.size());
+
+  EXPECT_EQ(minimizer_options.callbacks[0], &callback0);
+  EXPECT_EQ(minimizer_options.callbacks[1], &callback1);
+}
+
+}  // namespace internal
+}  // namespace ceres

internal/ceres/solver_impl.cc

@@ -132,12 +132,16 @@ void SolverImpl::Minimize(const Solver::Options& options,
   Minimizer::Options minimizer_options(options);
   LoggingCallback logging_callback(options.minimizer_progress_to_stdout);
   if (options.logging_type != SILENT) {
-    minimizer_options.callbacks.push_back(&logging_callback);
+    minimizer_options.callbacks.insert(minimizer_options.callbacks.begin(),
+                                       &logging_callback);
   }
 
   StateUpdatingCallback updating_callback(program, parameters);
   if (options.update_state_every_iteration) {
-    minimizer_options.callbacks.push_back(&updating_callback);
+    // This must get pushed to the front of the callbacks so that it is run
+    // before any of the user callbacks.
+    minimizer_options.callbacks.insert(minimizer_options.callbacks.begin(),
+                                       &updating_callback);
   }
 
   minimizer_options.evaluator = evaluator;

internal/ceres/solver_impl_test.cc

@@ -29,6 +29,7 @@
 // Author: sameeragarwal@google.com (Sameer Agarwal)
 
 #include "gtest/gtest.h"
+#include "ceres/autodiff_cost_function.h"
 #include "ceres/linear_solver.h"
 #include "ceres/parameter_block.h"
 #include "ceres/problem_impl.h"
@@ -560,5 +561,69 @@ TEST(SolverImpl, CreateLinearSolverNormalOperation) {
   EXPECT_TRUE(solver.get() != NULL);
 }
 
+struct QuadraticCostFunction {
+  template <typename T> bool operator()(const T* const x,
+                                        T* residual) const {
+    residual[0] = T(5.0) - *x;
+    return true;
+  }
+};
+
+struct RememberingCallback : public IterationCallback {
+  RememberingCallback(double *x) : calls(0), x(x) {}
+  virtual ~RememberingCallback() {}
+  virtual CallbackReturnType operator()(const IterationSummary& summary) {
+    x_values.push_back(*x);
+    return SOLVER_CONTINUE;
+  }
+  int calls;
+  double *x;
+  vector<double> x_values;
+};
+
+TEST(SolverImpl, UpdateStateEveryIterationOption) {
+  double x = 50.0;
+  const double original_x = x;
+
+  scoped_ptr<CostFunction> cost_function(
+      new AutoDiffCostFunction<QuadraticCostFunction, 1, 1>(
+          new QuadraticCostFunction));
+
+  Problem::Options problem_options;
+  problem_options.cost_function_ownership = DO_NOT_TAKE_OWNERSHIP;
+  Problem problem(problem_options);
+  problem.AddResidualBlock(cost_function.get(), NULL, &x);
+
+  Solver::Options options;
+  options.linear_solver_type = DENSE_QR;
+
+  RememberingCallback callback(&x);
+  options.callbacks.push_back(&callback);
+
+  Solver::Summary summary;
+
+  int num_iterations;
+
+  // First try: no updating.
+  SolverImpl::Solve(options, &problem, &summary);
+  num_iterations = summary.num_successful_steps +
+                   summary.num_unsuccessful_steps;
+  EXPECT_GT(num_iterations, 1);
+  for (int i = 0; i < callback.x_values.size(); ++i) {
+    EXPECT_EQ(50.0, callback.x_values[i]);
+  }
+
+  // Second try: with updating
+  x = 50.0;
+  options.update_state_every_iteration = true;
+  callback.x_values.clear();
+  SolverImpl::Solve(options, &problem, &summary);
+  num_iterations = summary.num_successful_steps +
+                   summary.num_unsuccessful_steps;
+  EXPECT_GT(num_iterations, 1);
+  EXPECT_EQ(original_x, callback.x_values[0]);
+  EXPECT_NE(original_x, callback.x_values[1]);
+}
+
 }  // namespace internal
 }  // namespace ceres

internal/ceres/system_test.cc

@@ -500,8 +500,8 @@ TEST(SystemTest, BundleAdjustmentProblem) {
 #endif  // CERES_NO_SUITESPARSE
 
 #ifndef CERES_NO_CXSPARSE
-  CONFIGURE(SPARSE_SCHUR,           CX_SPARSE, USER,    IDENTITY, 1);
-  CONFIGURE(SPARSE_SCHUR,           CX_SPARSE, SCHUR,   IDENTITY, 1);
+  CONFIGURE(SPARSE_SCHUR,           CX_SPARSE, USER,       IDENTITY, 1);
+  CONFIGURE(SPARSE_SCHUR,           CX_SPARSE, SCHUR,      IDENTITY, 1);
 #endif  // CERES_NO_CXSPARSE
 
   CONFIGURE(DENSE_SCHUR,            SUITE_SPARSE, USER,    IDENTITY, 1);

internal/ceres/trust_region_minimizer.cc

@@ -59,7 +59,6 @@ const double kEpsilon = 1e-12;
 // the callbacks does not return SOLVER_CONTINUE, then stop and return
 // its status.
 CallbackReturnType TrustRegionMinimizer::RunCallbacks(
-    const Minimizer::Options& options_,
     const IterationSummary& iteration_summary) {
   for (int i = 0; i < options_.callbacks.size(); ++i) {
     const CallbackReturnType status =
@@ -219,7 +218,7 @@ void TrustRegionMinimizer::Minimize(const Minimizer::Options& options,
   summary->iterations.push_back(iteration_summary);
 
   // Call the various callbacks.
-  switch (RunCallbacks(options_, iteration_summary)) {
+  switch (RunCallbacks(iteration_summary)) {
     case SOLVER_TERMINATE_SUCCESSFULLY:
       summary->termination_type = USER_SUCCESS;
       VLOG(1) << "Terminating: User callback returned USER_SUCCESS.";
@@ -441,7 +440,7 @@ void TrustRegionMinimizer::Minimize(const Minimizer::Options& options,
         summary->preprocessor_time_in_seconds;
     summary->iterations.push_back(iteration_summary);
 
-    switch (RunCallbacks(options_, iteration_summary)) {
+    switch (RunCallbacks(iteration_summary)) {
       case SOLVER_TERMINATE_SUCCESSFULLY:
         summary->termination_type = USER_SUCCESS;
         VLOG(1) << "Terminating: User callback returned USER_SUCCESS.";

internal/ceres/trust_region_minimizer.h

@@ -53,8 +53,7 @@ class TrustRegionMinimizer : public Minimizer {
  private:
   void Init(const Minimizer::Options& options);
   void EstimateScale(const SparseMatrix& jacobian, double* scale) const;
-  CallbackReturnType RunCallbacks(const Minimizer::Options& options,
-                                  const IterationSummary& iteration_summary);
+  CallbackReturnType RunCallbacks(const IterationSummary& iteration_summary);
   bool MaybeDumpLinearLeastSquaresProblem( const int iteration,
                                            const SparseMatrix* jacobian,
                                            const double* residuals,