From 2dd907744d83fd0bd3f58d1397be0a162f19a423 Mon Sep 17 00:00:00 2001 From: Sameer Agarwal Date: Sun, 15 Jan 2017 15:46:56 -0800 Subject: [PATCH] Ensure that partial evaluation of residuals triggers an error ResidualBlock evaluation has logic to ensure that CostFunction should always fill out the residual and jacobian arrays completely by using a special value to pre-populate these arrays. This works for CostFunctions with analytical Jacobians but not for AutoDiffCostFunction and NumericDiffCostFunction Jacobians. There is no way to fix this for NumericDiffCostFunctions without introducing significant performance penalties but the residual evaluation fails, which should be enough to catch such errors. For AutoDiffCostFunction the way the Jets are default initialized was sidestepping this check. So now, the Jet that is used to capture the output residuals is now initialized with kImpossibleValue, which will ensure that if the user forgets to fill all output fields, it triggers an evaluation error. This change required that ceres::internal::kImpossibleValue be moved out of array_utils.h/cc to types.h. Change-Id: I35bb0946cf0785a5d43c7b5459a2272848fb2a9b --- include/ceres/internal/autodiff.h | 7 +++++ include/ceres/types.h | 7 +++++ internal/ceres/array_utils.cc | 9 +----- internal/ceres/array_utils.h | 2 -- internal/ceres/autodiff_cost_function_test.cc | 26 ++++++++++++++++ .../ceres/numeric_diff_cost_function_test.cc | 31 +++++++++++++++++++ 6 files changed, 72 insertions(+), 10 deletions(-) diff --git a/include/ceres/internal/autodiff.h b/include/ceres/internal/autodiff.h index 136152a36..bf5fb774e 100644 --- a/include/ceres/internal/autodiff.h +++ b/include/ceres/internal/autodiff.h @@ -259,6 +259,13 @@ struct AutoDiff { JetT* output = x.get() + N0 + N1 + N2 + N3 + N4 + N5 + N6 + N7 + N8 + N9; + // Invalidate the output Jets, so that we can detect if the user + // did not assign values to all of them. + for (int i = 0; i < num_outputs; ++i) { + output[i].a = kImpossibleValue; + output[i].v.setConstant(kImpossibleValue); + } + #define CERES_MAKE_1ST_ORDER_PERTURBATION(i) \ if (N ## i) { \ internal::Make1stOrderPerturbation( \ diff --git a/include/ceres/types.h b/include/ceres/types.h index 2ea418036..6b9da8498 100644 --- a/include/ceres/types.h +++ b/include/ceres/types.h @@ -424,6 +424,13 @@ enum CovarianceAlgorithmType { EIGEN_SPARSE_QR }; +// It is a near impossibility that user code generates this exact +// value in normal operation, thus we will use it to fill arrays +// before passing them to user code. If on return an element of the +// array still contains this value, we will assume that the user code +// did not write to that memory location. +const double kImpossibleValue = 1e302; + CERES_EXPORT const char* LinearSolverTypeToString( LinearSolverType type); CERES_EXPORT bool StringToLinearSolverType(std::string value, diff --git a/internal/ceres/array_utils.cc b/internal/ceres/array_utils.cc index 7be3c78ce..a165a1bdf 100644 --- a/internal/ceres/array_utils.cc +++ b/internal/ceres/array_utils.cc @@ -37,19 +37,12 @@ #include #include "ceres/fpclassify.h" #include "ceres/stringprintf.h" - +#include "ceres/types.h" namespace ceres { namespace internal { using std::string; -// It is a near impossibility that user code generates this exact -// value in normal operation, thus we will use it to fill arrays -// before passing them to user code. If on return an element of the -// array still contains this value, we will assume that the user code -// did not write to that memory location. -const double kImpossibleValue = 1e302; - bool IsArrayValid(const int size, const double* x) { if (x != NULL) { for (int i = 0; i < size; ++i) { diff --git a/internal/ceres/array_utils.h b/internal/ceres/array_utils.h index 2d2ffca88..baf112e71 100644 --- a/internal/ceres/array_utils.h +++ b/internal/ceres/array_utils.h @@ -66,8 +66,6 @@ int FindInvalidValue(const int size, const double* x); // array pointer is NULL, it is treated as an array of zeros. void AppendArrayToString(const int size, const double* x, std::string* result); -extern const double kImpossibleValue; - // This routine takes an array of integer values, sorts and uniques // them and then maps each value in the array to its position in the // sorted+uniqued array. By doing this, if there are are k unique diff --git a/internal/ceres/autodiff_cost_function_test.cc b/internal/ceres/autodiff_cost_function_test.cc index 241fa0f67..d14fb8289 100644 --- a/internal/ceres/autodiff_cost_function_test.cc +++ b/internal/ceres/autodiff_cost_function_test.cc @@ -34,6 +34,7 @@ #include "gtest/gtest.h" #include "ceres/cost_function.h" +#include "ceres/array_utils.h" namespace ceres { namespace internal { @@ -142,5 +143,30 @@ TEST(AutodiffCostFunction, ManyParameterAutodiffInstantiates) { delete cost_function; } +struct OnlyFillsOneOutputFunctor { + template + bool operator()(const T* x, T* output) const { + output[0] = x[0]; + return true; + } +}; + +TEST(AutoDiffCostFunction, PartiallyFilledResidualShouldFailEvaluation) { + double parameter = 1.0; + double jacobian[2]; + double residuals[2]; + double* parameters[] = {¶meter}; + double* jacobians[] = {jacobian}; + + scoped_ptr cost_function( + new AutoDiffCostFunction( + new OnlyFillsOneOutputFunctor)); + InvalidateArray(2, jacobian); + InvalidateArray(2, residuals); + EXPECT_TRUE(cost_function->Evaluate(parameters, residuals, jacobians)); + EXPECT_FALSE(IsArrayValid(2, jacobian)); + EXPECT_FALSE(IsArrayValid(2, residuals)); +} + } // namespace internal } // namespace ceres diff --git a/internal/ceres/numeric_diff_cost_function_test.cc b/internal/ceres/numeric_diff_cost_function_test.cc index 13ab1069c..983f11e11 100644 --- a/internal/ceres/numeric_diff_cost_function_test.cc +++ b/internal/ceres/numeric_diff_cost_function_test.cc @@ -37,6 +37,7 @@ #include #include "ceres/internal/macros.h" #include "ceres/internal/scoped_ptr.h" +#include "ceres/array_utils.h" #include "ceres/numeric_diff_test_utils.h" #include "ceres/test_util.h" #include "ceres/types.h" @@ -353,6 +354,36 @@ TEST(NumericDiffCostFunction, RandomizedCostFunctionRidders) { functor.ExpectCostFunctionEvaluationIsNearlyCorrect(*cost_function); } +struct OnlyFillsOneOutputFunctor { + bool operator()(const double* x, double* output) const { + output[0] = x[0]; + return true; + } +}; + +TEST(NumericDiffCostFunction, PartiallyFilledResidualShouldFailEvaluation) { + double parameter = 1.0; + double jacobian[2]; + double residuals[2]; + double* parameters[] = {¶meter}; + double* jacobians[] = {jacobian}; + + scoped_ptr cost_function( + new NumericDiffCostFunction( + new OnlyFillsOneOutputFunctor)); + InvalidateArray(2, jacobian); + InvalidateArray(2, residuals); + EXPECT_TRUE(cost_function->Evaluate(parameters, residuals, jacobians)); + EXPECT_FALSE(IsArrayValid(2, residuals)); + InvalidateArray(2, residuals); + EXPECT_TRUE(cost_function->Evaluate(parameters, residuals, NULL)); + // We are only testing residuals here, because the Jacobians are + // computed using finite differencing from the residuals, so unless + // we introduce a validation step after every evaluation of + // residuals inside NumericDiffCostFunction, there is no way of + // ensuring that the Jacobian array is invalid. + EXPECT_FALSE(IsArrayValid(2, residuals)); +} } // namespace internal } // namespace ceres