theseus icon indicating copy to clipboard operation
theseus copied to clipboard

`dim` method in `CostFunction` should be consistent with the `error` dimension

Open joeaortiz opened this issue 3 years ago • 3 comments

🚀 Feature

CostFunction method dim should be consistent with the dimension of the error returned in the error method. This can be automatically satisfied by just returning self.error().shape[1] in the dim method.

Motivation

There can be inconsistencies between the cost function dim and the dim of the error if this is not automatically enforced.

Pitch

Alternatives

Additional context

joeaortiz avatar May 31 '22 16:05 joeaortiz

Do you have an example where dim is not consistent with the dimension of the error returned? I've fixed this error in some cost functions before, but maybe I missed some.

The reason we don't do it the way you propose is to avoid computing the error every time dim() is called, which can be a hefty cost. But one idea would be to compute it that way and save the value at construction time, which I think should work but would need to be tested.

luisenp avatar Jun 02 '22 11:06 luisenp

I came across it in the Reprojection cost function in the bundle adjustment example, where the dim is 2 but the error fn has shape batch x 1 : https://github.com/facebookresearch/theseus/blob/main/theseus/utils/examples/bundle_adjustment/reprojection_error.py#L58

Yeah agreed, it would be a very expensive way of computing the dim, your suggestion makes sense.

joeaortiz avatar Jun 03 '22 14:06 joeaortiz

Ah, I think this particular instance was fixed in experimental branch but never merged to main, if I recall correctly. Let's leave this feature request open until we add some sort of default for this. Thanks!

luisenp avatar Jun 03 '22 14:06 luisenp