From eafeca5dcb7af8688d40a9c14b0d2fcb856c96fc Mon Sep 17 00:00:00 2001 From: Sameer Agarwal Date: Mon, 28 Sep 2020 11:12:59 -0700 Subject: [PATCH] Fix a logging bug in TrustRegionMinimizer. Upon encountering an unsuccessful step (one where the cost goes up) the the trust region minimizer failed to populate the gradient norm in the IterationSummary. This would cause the gradient norm to be logged as zero which is incorrect. Instead it should be the gradient norm at the current point. This CL fixes this issue. Before: iter cost cost_change |gradient| |step| tr_ratio tr_radius ls_iter iter_time total_time 0 1.115206e+07 0.00e+00 1.90e+07 0.00e+00 0.00e+00 1.00e+04 0 2.72e-01 1.33e+00 1 3.687552e+06 7.46e+06 1.84e+08 2.86e+03 6.91e-01 1.06e+04 1 1.32e+00 2.65e+00 2 3.670266e+10 -3.67e+10 0.00e+00 3.27e+03 -1.07e+04 5.30e+03 1 7.52e-01 3.40e+00 3 4.335397e+07 -3.97e+07 0.00e+00 2.74e+03 -1.16e+01 1.32e+03 1 7.28e-01 4.13e+00 4 1.345488e+06 2.34e+06 4.12e+07 1.55e+03 6.87e-01 1.40e+03 1 9.31e-01 5.06e+00 5 5.376653e+05 8.08e+05 9.99e+06 6.64e+02 7.46e-01 1.59e+03 1 9.64e-01 6.03e+00 After: iter cost cost_change |gradient| |step| tr_ratio tr_radius ls_iter iter_time total_time 0 1.115206e+07 0.00e+00 1.90e+07 0.00e+00 0.00e+00 1.00e+04 0 2.37e-01 1.13e+00 1 3.687552e+06 7.46e+06 1.84e+08 2.86e+03 6.91e-01 1.06e+04 1 1.08e+00 2.21e+00 2 3.670266e+10 -3.67e+10 1.84e+08 3.27e+03 -1.07e+04 5.30e+03 1 7.50e-01 2.96e+00 3 4.335397e+07 -3.97e+07 1.84e+08 2.74e+03 -1.16e+01 1.32e+03 1 7.13e-01 3.67e+00 4 1.345488e+06 2.34e+06 4.12e+07 1.55e+03 6.87e-01 1.40e+03 1 9.01e-01 4.57e+00 5 5.376653e+05 8.08e+05 9.99e+06 6.64e+02 7.46e-01 1.59e+03 1 9.36e-01 5.51e+00 Change-Id: Iae538fe089be07c7bb219337a6f1392f7213acfe --- internal/ceres/trust_region_minimizer.cc | 27 +++++++++++++++--------- internal/ceres/trust_region_minimizer.h | 1 - 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/internal/ceres/trust_region_minimizer.cc b/internal/ceres/trust_region_minimizer.cc index 7065977ad..196f9e623 100644 --- a/internal/ceres/trust_region_minimizer.cc +++ b/internal/ceres/trust_region_minimizer.cc @@ -83,6 +83,10 @@ void TrustRegionMinimizer::Minimize(const Minimizer::Options& options, while (FinalizeIterationAndCheckIfMinimizerCanContinue()) { iteration_start_time_in_secs_ = WallTimeInSeconds(); + + const double previous_gradient_norm = iteration_summary_.gradient_norm; + const double previous_gradient_max_norm = iteration_summary_.gradient_max_norm; + iteration_summary_ = IterationSummary(); iteration_summary_.iteration = solver_summary->iterations.back().iteration + 1; @@ -111,12 +115,22 @@ void TrustRegionMinimizer::Minimize(const Minimizer::Options& options, return; } + + if (IsStepSuccessful()) { RETURN_IF_ERROR_AND_LOG(HandleSuccessfulStep()); - continue; - } + } else { + // Declare the step unsuccessful and inform the trust region strategy. + iteration_summary_.step_is_successful = false; + iteration_summary_.cost = candidate_cost_ + solver_summary_->fixed_cost; - HandleUnsuccessfulStep(); + // When the step is unsuccessful, we do not compute the gradient + // (or update x), so we preserve its value from the last + // successful iteration. + iteration_summary_.gradient_norm = previous_gradient_norm; + iteration_summary_.gradient_max_norm = previous_gradient_max_norm; + strategy_->StepRejected(iteration_summary_.relative_decrease); + } } } @@ -793,12 +807,5 @@ bool TrustRegionMinimizer::HandleSuccessfulStep() { return true; } -// Declare the step unsuccessful and inform the trust region strategy. -void TrustRegionMinimizer::HandleUnsuccessfulStep() { - iteration_summary_.step_is_successful = false; - strategy_->StepRejected(iteration_summary_.relative_decrease); - iteration_summary_.cost = candidate_cost_ + solver_summary_->fixed_cost; -} - } // namespace internal } // namespace ceres diff --git a/internal/ceres/trust_region_minimizer.h b/internal/ceres/trust_region_minimizer.h index e18193c92..27a1b7848 100644 --- a/internal/ceres/trust_region_minimizer.h +++ b/internal/ceres/trust_region_minimizer.h @@ -81,7 +81,6 @@ class TrustRegionMinimizer : public Minimizer { bool MinTrustRegionRadiusReached(); bool IsStepSuccessful(); - void HandleUnsuccessfulStep(); bool HandleSuccessfulStep(); bool HandleInvalidStep();