Check validity of residual block before removal in RemoveResidualBlock.
- Breaking change: Problem::Options::enable_fast_parameter_block_removal
is now Problem::Options::enable_fast_removal, as it now controls
the behaviour for both parameter and residual blocks.
- Previously we did not check that the specified residual block to
remove in RemoveResidualBlock actually represented a valid residual
for the problem.
- This meant that Ceres would die unexpectedly if the user passed an
uninitialised residual_block, or more likely attempted to remove a
residual block that had already been removed automatically after
the user removed a parameter block upon on which it was dependent.
- RemoveResidualBlock now verifies the validity of the given
residual_block to remove. Either by checking against a hash set of
all residuals maintained in ProblemImpl iff enable_fast_removal
is enabled. Or by a full scan of the residual blocks if not.
Change-Id: I9ab178e2f68a74135f0a8e20905b16405c77a62b
diff --git a/internal/ceres/problem_impl.cc b/internal/ceres/problem_impl.cc
index 99f3f89..d9fb2af 100644
--- a/internal/ceres/problem_impl.cc
+++ b/internal/ceres/problem_impl.cc
@@ -142,7 +142,7 @@
// For dynamic problems, add the list of dependent residual blocks, which is
// empty to start.
- if (options_.enable_fast_parameter_block_removal) {
+ if (options_.enable_fast_removal) {
new_parameter_block->EnableResidualBlockDependencies();
}
parameter_block_map_[values] = new_parameter_block;
@@ -150,6 +150,26 @@
return new_parameter_block;
}
+void ProblemImpl::InternalRemoveResidualBlock(ResidualBlock* residual_block) {
+ CHECK_NOTNULL(residual_block);
+ // Perform no check on the validity of residual_block, that is handled in
+ // the public method: RemoveResidualBlock().
+
+ // If needed, remove the parameter dependencies on this residual block.
+ if (options_.enable_fast_removal) {
+ const int num_parameter_blocks_for_residual =
+ residual_block->NumParameterBlocks();
+ for (int i = 0; i < num_parameter_blocks_for_residual; ++i) {
+ residual_block->parameter_blocks()[i]
+ ->RemoveResidualBlock(residual_block);
+ }
+
+ ResidualBlockSet::iterator it = residual_block_set_.find(residual_block);
+ residual_block_set_.erase(it);
+ }
+ DeleteBlockInVector(program_->mutable_residual_blocks(), residual_block);
+}
+
// Deletes the residual block in question, assuming there are no other
// references to it inside the problem (e.g. by another parameter). Referenced
// cost and loss functions are tucked away for future deletion, since it is not
@@ -278,13 +298,18 @@
program_->residual_blocks_.size());
// Add dependencies on the residual to the parameter blocks.
- if (options_.enable_fast_parameter_block_removal) {
+ if (options_.enable_fast_removal) {
for (int i = 0; i < parameter_blocks.size(); ++i) {
parameter_block_ptrs[i]->AddResidualBlock(new_residual_block);
}
}
program_->residual_blocks_.push_back(new_residual_block);
+
+ if (options_.enable_fast_removal) {
+ residual_block_set_.insert(new_residual_block);
+ }
+
return new_residual_block;
}
@@ -475,30 +500,46 @@
void ProblemImpl::RemoveResidualBlock(ResidualBlock* residual_block) {
CHECK_NOTNULL(residual_block);
- // If needed, remove the parameter dependencies on this residual block.
- if (options_.enable_fast_parameter_block_removal) {
- const int num_parameter_blocks_for_residual =
- residual_block->NumParameterBlocks();
- for (int i = 0; i < num_parameter_blocks_for_residual; ++i) {
- residual_block->parameter_blocks()[i]
- ->RemoveResidualBlock(residual_block);
- }
+ // Verify that residual_block identifies a residual in the current problem.
+ const string residual_not_found_message =
+ StringPrintf("Residual block to remove: %p not found. This usually means "
+ "one of three things have happened:\n"
+ " 1) residual_block is uninitialised and points to a random "
+ "area in memory.\n"
+ " 2) residual_block represented a residual that was added to"
+ " the problem, but referred to a parameter block which has "
+ "since been removed, which removes all residuals which "
+ "depend on that parameter block, and was thus removed.\n"
+ " 3) residual_block referred to a residual that has already "
+ "been removed from the problem (by the user).",
+ residual_block);
+ if (options_.enable_fast_removal) {
+ CHECK(residual_block_set_.find(residual_block) !=
+ residual_block_set_.end())
+ << residual_not_found_message;
+ } else {
+ // Perform a full search over all current residuals.
+ CHECK(std::find(program_->residual_blocks().begin(),
+ program_->residual_blocks().end(),
+ residual_block) != program_->residual_blocks().end())
+ << residual_not_found_message;
}
- DeleteBlockInVector(program_->mutable_residual_blocks(), residual_block);
+
+ InternalRemoveResidualBlock(residual_block);
}
void ProblemImpl::RemoveParameterBlock(double* values) {
ParameterBlock* parameter_block =
FindParameterBlockOrDie(parameter_block_map_, values);
- if (options_.enable_fast_parameter_block_removal) {
+ if (options_.enable_fast_removal) {
// Copy the dependent residuals from the parameter block because the set of
// dependents will change after each call to RemoveResidualBlock().
vector<ResidualBlock*> residual_blocks_to_remove(
parameter_block->mutable_residual_blocks()->begin(),
parameter_block->mutable_residual_blocks()->end());
for (int i = 0; i < residual_blocks_to_remove.size(); ++i) {
- RemoveResidualBlock(residual_blocks_to_remove[i]);
+ InternalRemoveResidualBlock(residual_blocks_to_remove[i]);
}
} else {
// Scan all the residual blocks to remove ones that depend on the parameter
@@ -510,7 +551,7 @@
const int num_parameter_blocks = residual_block->NumParameterBlocks();
for (int j = 0; j < num_parameter_blocks; ++j) {
if (residual_block->parameter_blocks()[j] == parameter_block) {
- RemoveResidualBlock(residual_block);
+ InternalRemoveResidualBlock(residual_block);
// The parameter blocks are guaranteed unique.
break;
}
@@ -784,7 +825,7 @@
FindParameterBlockOrDie(parameter_block_map_,
const_cast<double*>(values));
- if (options_.enable_fast_parameter_block_removal) {
+ if (options_.enable_fast_removal) {
// In this case the residual blocks that depend on the parameter block are
// stored in the parameter block already, so just copy them out.
CHECK_NOTNULL(residual_blocks)->resize(
