fix: arg type for std::ceil and typo in glomap::EstimateRelativePoses #94

lnexenl · 2024-09-06T10:30:34Z

I find this bug when reconstructing 4 images. std::ceil(num_image_pairs / kNumChunks) equals to zero at here for int64(4/10) equals zero, and the relative pose estimation actually failed here.

ahojnnes · 2024-09-09T15:58:07Z

Thanks for catching this bug. LGTM.

ahojnnes · 2024-09-09T16:01:20Z

glomap/estimators/relpose_estimation.cc

@@ -20,14 +20,15 @@ void EstimateRelativePoses(ViewGraph& view_graph,

  const int64_t num_image_pairs = valid_pair_ids.size();
  const int64_t kNumChunks = 10;
-  const int64_t inverval = std::ceil(num_image_pairs / kNumChunks);
+  const int64_t interval =
+      std::ceil(static_cast<double>(num_image_pairs) / kNumChunks);


Suggested change

std::ceil(static_cast<double>(num_image_pairs) / kNumChunks);

std::min(num_image_pairs / kNumChunks, 1);

nit: this would be a safer alternative that handles the full range of int64 numbers while double may not hold the full range of int64 numbers. This is unrealistic though, because this for loop would probably run for a very very long time.

Thanks so much! I really did not take this problem into account. But as you say, I think there won't be anyone processing so many pictures. 🤣

fix: arg type for std::ceil and typo in glomap::EstimateRelativePoses

bb325b1

ahojnnes approved these changes Sep 9, 2024

View reviewed changes

ahojnnes enabled auto-merge (squash) September 9, 2024 15:58

ahojnnes reviewed Sep 9, 2024

View reviewed changes

ahojnnes merged commit 410fc07 into colmap:main Sep 9, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: arg type for std::ceil and typo in glomap::EstimateRelativePoses #94

fix: arg type for std::ceil and typo in glomap::EstimateRelativePoses #94

	std::ceil(static_cast<double>(num_image_pairs) / kNumChunks);
	std::min(num_image_pairs / kNumChunks, 1);

fix: arg type for std::ceil and typo in glomap::EstimateRelativePoses #94

fix: arg type for std::ceil and typo in glomap::EstimateRelativePoses #94

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment