Throw exception at counting registered images when an image in the frame does not exist in the reconstruction.

[B1ueber2y]

Follow up to #3540 and #3543

[ahojnnes]

I may have misunderstood the issue before, but I don't think we should support images in frames that are not part of the reconstruction. What's the use case for this?

[B1ueber2y]

I may have misunderstood the issue before, but I don't think we should support images in frames that are not part of the reconstruction. What's the use case for this?

There are no explicit use cases. The fix is more for the completeness of the reconstruction data structure, since there is no way to enforce that the images in the frames are all added in our current design. We don't want NumRegImages() and RegImageIds() to be broken in certain state of reconstruction object.

[B1ueber2y]

So this is a safety improvement, similar to the previous two PRs.

[ahojnnes]

I'd rather have an exception thrown when the images were not added rather than silently skipping over those images, as implemented in this PR... If we want to add some extra safety, we could do another loop in the Reconstruction::Load() method to check that all images in the frames actually exist?

[B1ueber2y]

I'd rather have an exception thrown when the images were not added rather than silently skipping over those images, as implemented in this PR... If we want to add some extra safety, we could do another loop in the Reconstruction::Load() method to check that all images in the frames actually exist?

Since we expose the Reconstruction class to public, Load() is not the only way to create a Reconstruction object. People can do it in the same way as how we do to create test cases, or construct it via pycolmap (this is how i found all these safety issues since some of the pycolmap construction code (e.g., #3522) can miss pieces but no error will be given).

[B1ueber2y]

Because AddImage is not forced when we do AddFrame, there is no way to enforce the images to be added, and there is no clear place to check and raise the exception. With the current design, I think we would need to live with such Reconstruction object.

[ahojnnes]

Yeah, I understand, but then somebody is misusing the contract. Silently ignoring errors in the setup is not the right solution. Let's make sure, if somebody misconfigures the reconstruction, they will see a meaningful error message. I thought your previous PRs would address this?

[B1ueber2y]

Yeah, I understand, but then somebody is misusing the contract. Silently ignoring errors in the setup is not the right solution. Let's make sure, if somebody misconfigures the reconstruction, they will see a meaningful error message. I thought your previous PRs would address this?

Yes. Most of the errors are addressed by my previous PRs, except for that we cannot check whether all images in the frame are added, because AddImage happens after AddFrame, which leads to the proposal in this PR.

[ahojnnes]

Yeah, I understand, but then somebody is misusing the contract. Silently ignoring errors in the setup is not the right solution. Let's make sure, if somebody misconfigures the reconstruction, they will see a meaningful error message. I thought your previous PRs would address this?

Yes. Most of the errors are addressed by my previous PRs, except for that we cannot check whether all images in the frame are added, because AddImage happens after AddFrame, which leads to the proposal in this PR.

Thanks, I think that is fine as long as subsequent operations on the reconstruction / frame / image objects lead to obvious errors, so people can fix the issue. The constraints in the SQLite schema should already prevent this when people manually populate objects in the database.

[B1ueber2y]

Yeah, I understand, but then somebody is misusing the contract. Silently ignoring errors in the setup is not the right solution. Let's make sure, if somebody misconfigures the reconstruction, they will see a meaningful error message. I thought your previous PRs would address this?

Yes. Most of the errors are addressed by my previous PRs, except for that we cannot check whether all images in the frame are added, because AddImage happens after AddFrame, which leads to the proposal in this PR.

Thanks, I think that is fine as long as subsequent operations on the reconstruction / frame / image objects lead to obvious errors, so people can fix the issue. The constraints in the SQLite schema should already prevent this when people manually populate objects in the database.

Should I then change all the "continue;" in this PR to throwing exceptions?

[ahojnnes]

I don't think that adds any value. The reconstruction.Image(data_id.id); will already throw with a meaningful error message when the image does not exist?

[ahojnnes]

The only place where it could be useful is where we count/accum the registered images?

[B1ueber2y]

Sure. Then i will just throw at NumRegImages and RegImageIds()?

[B1ueber2y]

Sure. Then i will just throw at NumRegImages and RegImageIds()?

Done. PTAL : )

[ahojnnes]

Thanks, looks great.