Rewrite uses of VLOG_IF and LOG_IF.
VLOG_IF's evaluation order is ambiguous - does it mean
`if (cond) VLOG(lvl)` or `if (VLOG_IS_ON(lvl) && cond) LOG(INFO)`?
In particular, the way it works now is inconsistent with the way the
rest of the LOG macros evaluate their arguments.
Fixing this would be hard, and the macro's behavior would still surprise
some people. Replacing it with an if statement is simple, clear, and unambiguous.
Change-Id: I97a92d17a932c0a5344a1bf98d676308793ba877
diff --git a/internal/ceres/line_search.cc b/internal/ceres/line_search.cc
index 3f5103f..7e871a2 100644
--- a/internal/ceres/line_search.cc
+++ b/internal/ceres/line_search.cc
@@ -324,7 +324,9 @@
"satisfying the sufficient decrease condition within "
"specified max_num_iterations: %d.",
options().max_num_iterations);
- LOG_IF(WARNING, !options().is_silent) << summary->error;
+ if (!options().is_silent) {
+ LOG(WARNING) << summary->error;
+ }
return;
}
@@ -345,7 +347,9 @@
"with descent_direction_max_norm: %.5e.",
step_size,
descent_direction_max_norm);
- LOG_IF(WARNING, !options().is_silent) << summary->error;
+ if (!options().is_silent) {
+ LOG(WARNING) << summary->error;
+ }
return;
}
@@ -583,16 +587,18 @@
// conditions, or a valid bracket containing such a point. Stop searching
// and set bracket_low to the size size amongst all those tested which
// minimizes f() and satisfies the Armijo condition.
- LOG_IF(WARNING, !options().is_silent)
- << "Line search failed: Wolfe bracketing phase shrank "
- << "bracket width: " << fabs(current.x - previous.x)
- << ", to < tolerance: " << options().min_step_size
- << ", with descent_direction_max_norm: " << descent_direction_max_norm
- << ", and failed to find "
- << "a point satisfying the strong Wolfe conditions or a "
- << "bracketing containing such a point. Accepting "
- << "point found satisfying Armijo condition only, to "
- << "allow continuation.";
+
+ if (!options().is_silent) {
+ LOG(WARNING) << "Line search failed: Wolfe bracketing phase shrank "
+ << "bracket width: " << fabs(current.x - previous.x)
+ << ", to < tolerance: " << options().min_step_size
+ << ", with descent_direction_max_norm: "
+ << descent_direction_max_norm << ", and failed to find "
+ << "a point satisfying the strong Wolfe conditions or a "
+ << "bracketing containing such a point. Accepting "
+ << "point found satisfying Armijo condition only, to "
+ << "allow continuation.";
+ }
*bracket_low = current;
break;
@@ -606,7 +612,9 @@
"bracket containing such a point within specified "
"max_num_iterations: %d",
options().max_num_iterations);
- LOG_IF(WARNING, !options().is_silent) << summary->error;
+ if (!options().is_silent) {
+ LOG(WARNING) << summary->error;
+ }
// Ensure that bracket_low is always set to the step size amongst all
// those tested which minimizes f() and satisfies the Armijo condition
// when we terminate due to the 'artificial' max_num_iterations condition.
@@ -659,7 +667,9 @@
"with descent_direction_max_norm: %.5e",
step_size,
descent_direction_max_norm);
- LOG_IF(WARNING, !options().is_silent) << summary->error;
+ if (!options().is_silent) {
+ LOG(WARNING) << summary->error;
+ }
return false;
}
@@ -739,7 +749,9 @@
initial_position.ToDebugString().c_str(),
bracket_low.ToDebugString().c_str(),
bracket_high.ToDebugString().c_str());
- LOG_IF(WARNING, !options().is_silent) << summary->error;
+ if (!options().is_silent) {
+ LOG(WARNING) << summary->error;
+ }
solution->value_is_valid = false;
return false;
}
@@ -760,7 +772,9 @@
"(num iterations taken for bracketing: %d).",
options().max_num_iterations,
num_bracketing_iterations);
- LOG_IF(WARNING, !options().is_silent) << summary->error;
+ if (!options().is_silent) {
+ LOG(WARNING) << summary->error;
+ }
return false;
}
if (fabs(bracket_high.x - bracket_low.x) * descent_direction_max_norm <
@@ -772,7 +786,9 @@
"too small with descent_direction_max_norm: %.5e.",
fabs(bracket_high.x - bracket_low.x),
descent_direction_max_norm);
- LOG_IF(WARNING, !options().is_silent) << summary->error;
+ if (!options().is_silent) {
+ LOG(WARNING) << summary->error;
+ }
return false;
}
@@ -824,7 +840,9 @@
solution->x,
bracket_low.x,
bracket_high.x);
- LOG_IF(WARNING, !options().is_silent) << summary->error;
+ if (!options().is_silent) {
+ LOG(WARNING) << summary->error;
+ }
return false;
}
diff --git a/internal/ceres/line_search_direction.cc b/internal/ceres/line_search_direction.cc
index 74e9d91..48e6c98 100644
--- a/internal/ceres/line_search_direction.cc
+++ b/internal/ceres/line_search_direction.cc
@@ -148,12 +148,14 @@
use_approximate_eigenvalue_scaling_(use_approximate_eigenvalue_scaling),
initialized_(false),
is_positive_definite_(true) {
- LOG_IF(WARNING, num_parameters_ >= 1e3)
- << "BFGS line search being created with: " << num_parameters_
- << " parameters, this will allocate a dense approximate inverse Hessian"
- << " of size: " << num_parameters_ << " x " << num_parameters_
- << ", consider using the L-BFGS memory-efficient line search direction "
- << "instead.";
+ if (num_parameters_ >= 1000) {
+ LOG(WARNING) << "BFGS line search being created with: " << num_parameters_
+ << " parameters, this will allocate a dense approximate "
+ << "inverse Hessian of size: " << num_parameters_ << " x "
+ << num_parameters_
+ << ", consider using the L-BFGS memory-efficient line "
+ << "search direction instead.";
+ }
// Construct inverse_hessian_ after logging warning about size s.t. if the
// allocation crashes us, the log will highlight what the issue likely was.
inverse_hessian_ = Matrix::Identity(num_parameters, num_parameters);
diff --git a/internal/ceres/line_search_minimizer.cc b/internal/ceres/line_search_minimizer.cc
index 5fed73c..ea1c507 100644
--- a/internal/ceres/line_search_minimizer.cc
+++ b/internal/ceres/line_search_minimizer.cc
@@ -123,7 +123,9 @@
nullptr)) {
summary->termination_type = FAILURE;
summary->message = "Initial cost and jacobian evaluation failed.";
- LOG_IF(WARNING, is_not_silent) << "Terminating: " << summary->message;
+ if (is_not_silent) {
+ LOG(WARNING) << "Terminating: " << summary->message;
+ }
return;
}
@@ -132,7 +134,9 @@
summary->message =
"Initial cost and jacobian evaluation failed. More details: " +
summary->message;
- LOG_IF(WARNING, is_not_silent) << "Terminating: " << summary->message;
+ if (is_not_silent) {
+ LOG(WARNING) << "Terminating: " << summary->message;
+ }
return;
}
@@ -147,7 +151,9 @@
iteration_summary.gradient_max_norm,
options.gradient_tolerance);
summary->termination_type = CONVERGENCE;
- VLOG_IF(1, is_not_silent) << "Terminating: " << summary->message;
+ if (is_not_silent) {
+ VLOG(1) << "Terminating: " << summary->message;
+ }
return;
}
@@ -193,7 +199,9 @@
options.line_search_type, line_search_options, &summary->message));
if (line_search.get() == nullptr) {
summary->termination_type = FAILURE;
- LOG_IF(ERROR, is_not_silent) << "Terminating: " << summary->message;
+ if (is_not_silent) {
+ LOG(ERROR) << "Terminating: " << summary->message;
+ }
return;
}
@@ -209,7 +217,9 @@
if (iteration_summary.iteration >= options.max_num_iterations) {
summary->message = "Maximum number of iterations reached.";
summary->termination_type = NO_CONVERGENCE;
- VLOG_IF(1, is_not_silent) << "Terminating: " << summary->message;
+ if (is_not_silent) {
+ VLOG(1) << "Terminating: " << summary->message;
+ }
break;
}
@@ -218,7 +228,9 @@
if (total_solver_time >= options.max_solver_time_in_seconds) {
summary->message = "Maximum solver time reached.";
summary->termination_type = NO_CONVERGENCE;
- VLOG_IF(1, is_not_silent) << "Terminating: " << summary->message;
+ if (is_not_silent) {
+ VLOG(1) << "Terminating: " << summary->message;
+ }
break;
}
@@ -246,7 +258,9 @@
"max_num_line_search_direction_restarts: %d reached.",
options.max_num_line_search_direction_restarts);
summary->termination_type = FAILURE;
- LOG_IF(WARNING, is_not_silent) << "Terminating: " << summary->message;
+ if (is_not_silent) {
+ LOG(WARNING) << "Terminating: " << summary->message;
+ }
break;
} else if (!line_search_status) {
// Restart line search direction with gradient descent on first iteration
@@ -255,14 +269,17 @@
options.max_num_line_search_direction_restarts);
++num_line_search_direction_restarts;
- LOG_IF(WARNING, is_not_silent)
- << "Line search direction algorithm: "
- << LineSearchDirectionTypeToString(options.line_search_direction_type)
- << ", failed to produce a valid new direction at "
- << "iteration: " << iteration_summary.iteration
- << ". Restarting, number of restarts: "
- << num_line_search_direction_restarts << " / "
- << options.max_num_line_search_direction_restarts << " [max].";
+ if (is_not_silent) {
+ LOG(WARNING) << "Line search direction algorithm: "
+ << LineSearchDirectionTypeToString(
+ options.line_search_direction_type)
+ << ", failed to produce a valid new direction at "
+ << "iteration: " << iteration_summary.iteration
+ << ". Restarting, number of restarts: "
+ << num_line_search_direction_restarts << " / "
+ << options.max_num_line_search_direction_restarts
+ << " [max].";
+ }
line_search_direction.reset(
LineSearchDirection::Create(line_search_direction_options));
current_state.search_direction = -current_state.gradient;
@@ -296,7 +313,9 @@
current_state.directional_derivative,
(current_state.cost - previous_state.cost));
summary->termination_type = FAILURE;
- LOG_IF(WARNING, is_not_silent) << "Terminating: " << summary->message;
+ if (is_not_silent) {
+ LOG(WARNING) << "Terminating: " << summary->message;
+ }
break;
}
@@ -313,7 +332,9 @@
initial_step_size,
current_state.cost,
current_state.directional_derivative);
- LOG_IF(WARNING, is_not_silent) << "Terminating: " << summary->message;
+ if (is_not_silent) {
+ LOG(WARNING) << "Terminating: " << summary->message;
+ }
summary->termination_type = FAILURE;
break;
}
@@ -340,7 +361,9 @@
nullptr)) {
summary->termination_type = FAILURE;
summary->message = "Cost and jacobian evaluation failed.";
- LOG_IF(WARNING, is_not_silent) << "Terminating: " << summary->message;
+ if (is_not_silent) {
+ LOG(WARNING) << "Terminating: " << summary->message;
+ }
return;
}
}
@@ -354,7 +377,9 @@
"Step failed to evaluate. This should not happen as the step was "
"valid when it was selected by the line search. More details: " +
summary->message;
- LOG_IF(WARNING, is_not_silent) << "Terminating: " << summary->message;
+ if (is_not_silent) {
+ LOG(WARNING) << "Terminating: " << summary->message;
+ }
break;
}
@@ -411,7 +436,9 @@
(x_norm + options.parameter_tolerance)),
options.parameter_tolerance);
summary->termination_type = CONVERGENCE;
- VLOG_IF(1, is_not_silent) << "Terminating: " << summary->message;
+ if (is_not_silent) {
+ VLOG(1) << "Terminating: " << summary->message;
+ }
return;
}
@@ -422,7 +449,9 @@
iteration_summary.gradient_max_norm,
options.gradient_tolerance);
summary->termination_type = CONVERGENCE;
- VLOG_IF(1, is_not_silent) << "Terminating: " << summary->message;
+ if (is_not_silent) {
+ VLOG(1) << "Terminating: " << summary->message;
+ }
break;
}
@@ -436,7 +465,9 @@
std::abs(iteration_summary.cost_change) / previous_state.cost,
options.function_tolerance);
summary->termination_type = CONVERGENCE;
- VLOG_IF(1, is_not_silent) << "Terminating: " << summary->message;
+ if (is_not_silent) {
+ VLOG(1) << "Terminating: " << summary->message;
+ }
break;
}
}
diff --git a/internal/ceres/minimizer.cc b/internal/ceres/minimizer.cc
index 4943a75..b96e0c9 100644
--- a/internal/ceres/minimizer.cc
+++ b/internal/ceres/minimizer.cc
@@ -70,12 +70,16 @@
summary->termination_type = USER_SUCCESS;
summary->message =
"User callback returned SOLVER_TERMINATE_SUCCESSFULLY.";
- VLOG_IF(1, is_not_silent) << "Terminating: " << summary->message;
+ if (is_not_silent) {
+ VLOG(1) << "Terminating: " << summary->message;
+ }
return false;
case SOLVER_ABORT:
summary->termination_type = USER_FAILURE;
summary->message = "User callback returned SOLVER_ABORT.";
- VLOG_IF(1, is_not_silent) << "Terminating: " << summary->message;
+ if (is_not_silent) {
+ VLOG(1) << "Terminating: " << summary->message;
+ }
return false;
default:
LOG(FATAL) << "Unknown type of user callback status";
diff --git a/internal/ceres/problem_impl.cc b/internal/ceres/problem_impl.cc
index 20da652..3155bc3 100644
--- a/internal/ceres/problem_impl.cc
+++ b/internal/ceres/problem_impl.cc
@@ -685,10 +685,12 @@
// type of linear solver being used.
evaluator_options.linear_solver_type = SPARSE_NORMAL_CHOLESKY;
#ifdef CERES_NO_THREADS
- LOG_IF(WARNING, evaluate_options.num_threads > 1)
- << "No threading support is compiled into this binary; "
- << "only evaluate_options.num_threads = 1 is supported. Switching "
- << "to single threaded mode.";
+ if (evaluate_options.num_threads > 1) {
+ LOG(WARNING)
+ << "No threading support is compiled into this binary; "
+ << "only evaluate_options.num_threads = 1 is supported. Switching "
+ << "to single threaded mode.";
+ }
evaluator_options.num_threads = 1;
#else
evaluator_options.num_threads = evaluate_options.num_threads;
diff --git a/internal/ceres/solver.cc b/internal/ceres/solver.cc
index b7399e6..dfde122 100644
--- a/internal/ceres/solver.cc
+++ b/internal/ceres/solver.cc
@@ -267,17 +267,18 @@
// on max/min step size change during line search prevent bisection scaling
// from occurring. Warn only, as this is likely a user mistake, but one which
// does not prevent us from continuing.
- LOG_IF(WARNING,
- (options.line_search_interpolation_type == ceres::BISECTION &&
- (options.max_line_search_step_contraction > 0.5 ||
- options.min_line_search_step_contraction < 0.5)))
- << "Line search interpolation type is BISECTION, but specified "
- << "max_line_search_step_contraction: "
- << options.max_line_search_step_contraction << ", and "
- << "min_line_search_step_contraction: "
- << options.min_line_search_step_contraction
- << ", prevent bisection (0.5) scaling, continuing with solve regardless.";
-
+ if (options.line_search_interpolation_type == ceres::BISECTION &&
+ (options.max_line_search_step_contraction > 0.5 ||
+ options.min_line_search_step_contraction < 0.5)) {
+ LOG(WARNING)
+ << "Line search interpolation type is BISECTION, but specified "
+ << "max_line_search_step_contraction: "
+ << options.max_line_search_step_contraction << ", and "
+ << "min_line_search_step_contraction: "
+ << options.min_line_search_step_contraction
+ << ", prevent bisection (0.5) scaling, continuing with solve "
+ "regardless.";
+ }
return true;
}
@@ -426,7 +427,9 @@
"Function tolerance reached. "
"No non-constant parameter blocks found.";
summary->termination_type = CONVERGENCE;
- VLOG_IF(1, pp->options.logging_type != SILENT) << summary->message;
+ if (pp->options.logging_type != SILENT) {
+ VLOG(1) << summary->message;
+ }
summary->initial_cost = summary->fixed_cost;
summary->final_cost = summary->fixed_cost;
return;
diff --git a/internal/ceres/trust_region_minimizer.cc b/internal/ceres/trust_region_minimizer.cc
index 96334de..bcf05b3 100644
--- a/internal/ceres/trust_region_minimizer.cc
+++ b/internal/ceres/trust_region_minimizer.cc
@@ -438,10 +438,12 @@
num_consecutive_invalid_steps_ = 0;
}
- VLOG_IF(1, is_not_silent_ && !iteration_summary_.step_is_valid)
- << "Invalid step: current_cost: " << x_cost_
- << " absolute model cost change: " << model_cost_change_
- << " relative model cost change: " << (model_cost_change_ / x_cost_);
+ if (is_not_silent_ && !iteration_summary_.step_is_valid) {
+ VLOG(1) << "Invalid step: current_cost: " << x_cost_
+ << " absolute model cost change: " << model_cost_change_
+ << " relative model cost change: "
+ << (model_cost_change_ / x_cost_);
+ }
return true;
}
@@ -509,15 +511,17 @@
nullptr,
nullptr,
nullptr)) {
- VLOG_IF(2, is_not_silent_) << "Inner iteration failed.";
+ if (is_not_silent_) {
+ VLOG(2) << "Inner iteration failed.";
+ }
return;
}
- VLOG_IF(2, is_not_silent_)
- << "Inner iteration succeeded; Current cost: " << x_cost_
- << " Trust region step cost: " << candidate_cost_
- << " Inner iteration cost: " << inner_iteration_cost;
-
+ if (is_not_silent_) {
+ VLOG(2) << "Inner iteration succeeded; Current cost: " << x_cost_
+ << " Trust region step cost: " << candidate_cost_
+ << " Inner iteration cost: " << inner_iteration_cost;
+ }
candidate_x_ = inner_iteration_x_;
// Normally, the quality of a trust region step is measured by
@@ -552,9 +556,10 @@
// drops below tolerance.
inner_iterations_are_enabled_ =
(inner_iteration_relative_progress > options_.inner_iteration_tolerance);
- VLOG_IF(2, is_not_silent_ && !inner_iterations_are_enabled_)
- << "Disabling inner iterations. Progress : "
- << inner_iteration_relative_progress;
+ if (is_not_silent_ && !inner_iterations_are_enabled_) {
+ VLOG(2) << "Disabling inner iterations. Progress : "
+ << inner_iteration_relative_progress;
+ }
candidate_cost_ = inner_iteration_cost;
solver_summary_->inner_iteration_time_in_seconds +=
@@ -632,7 +637,9 @@
total_solver_time,
options_.max_solver_time_in_seconds);
solver_summary_->termination_type = NO_CONVERGENCE;
- VLOG_IF(1, is_not_silent_) << "Terminating: " << solver_summary_->message;
+ if (is_not_silent_) {
+ VLOG(1) << "Terminating: " << solver_summary_->message;
+ }
return true;
}
@@ -650,7 +657,9 @@
iteration_summary_.iteration);
solver_summary_->termination_type = NO_CONVERGENCE;
- VLOG_IF(1, is_not_silent_) << "Terminating: " << solver_summary_->message;
+ if (is_not_silent_) {
+ VLOG(1) << "Terminating: " << solver_summary_->message;
+ }
return true;
}
@@ -668,7 +677,9 @@
iteration_summary_.gradient_max_norm,
options_.gradient_tolerance);
solver_summary_->termination_type = CONVERGENCE;
- VLOG_IF(1, is_not_silent_) << "Terminating: " << solver_summary_->message;
+ if (is_not_silent_) {
+ VLOG(1) << "Terminating: " << solver_summary_->message;
+ }
return true;
}
@@ -685,7 +696,9 @@
iteration_summary_.trust_region_radius,
options_.min_trust_region_radius);
solver_summary_->termination_type = CONVERGENCE;
- VLOG_IF(1, is_not_silent_) << "Terminating: " << solver_summary_->message;
+ if (is_not_silent_) {
+ VLOG(1) << "Terminating: " << solver_summary_->message;
+ }
return true;
}
@@ -706,7 +719,9 @@
(iteration_summary_.step_norm / (x_norm_ + options_.parameter_tolerance)),
options_.parameter_tolerance);
solver_summary_->termination_type = CONVERGENCE;
- VLOG_IF(1, is_not_silent_) << "Terminating: " << solver_summary_->message;
+ if (is_not_silent_) {
+ VLOG(1) << "Terminating: " << solver_summary_->message;
+ }
return true;
}
@@ -726,7 +741,9 @@
fabs(iteration_summary_.cost_change) / x_cost_,
options_.function_tolerance);
solver_summary_->termination_type = CONVERGENCE;
- VLOG_IF(1, is_not_silent_) << "Terminating: " << solver_summary_->message;
+ if (is_not_silent_) {
+ VLOG(1) << "Terminating: " << solver_summary_->message;
+ }
return true;
}
@@ -743,18 +760,20 @@
// CostFunction objects.
void TrustRegionMinimizer::ComputeCandidatePointAndEvaluateCost() {
if (!evaluator_->Plus(x_.data(), delta_.data(), candidate_x_.data())) {
- LOG_IF(WARNING, is_not_silent_)
- << "x_plus_delta = Plus(x, delta) failed. "
- << "Treating it as a step with infinite cost";
+ if (is_not_silent_) {
+ LOG(WARNING) << "x_plus_delta = Plus(x, delta) failed. "
+ << "Treating it as a step with infinite cost";
+ }
candidate_cost_ = std::numeric_limits<double>::max();
return;
}
if (!evaluator_->Evaluate(
candidate_x_.data(), &candidate_cost_, nullptr, nullptr, nullptr)) {
- LOG_IF(WARNING, is_not_silent_)
- << "Step failed to evaluate. "
- << "Treating it as a step with infinite cost";
+ if (is_not_silent_) {
+ LOG(WARNING) << "Step failed to evaluate. "
+ << "Treating it as a step with infinite cost";
+ }
candidate_cost_ = std::numeric_limits<double>::max();
}
}
diff --git a/internal/ceres/trust_region_preprocessor.cc b/internal/ceres/trust_region_preprocessor.cc
index b8c6b49..0943edb 100644
--- a/internal/ceres/trust_region_preprocessor.cc
+++ b/internal/ceres/trust_region_preprocessor.cc
@@ -101,8 +101,9 @@
LinearSolverTypeToString(linear_solver_type_given),
LinearSolverTypeToString(options->linear_solver_type));
}
-
- VLOG_IF(1, options->logging_type != SILENT) << message;
+ if (options->logging_type != SILENT) {
+ VLOG(1) << message;
+ }
}
// Reorder the program to reduce fill-in and increase cache coherency.
