Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

find_essential and run_5point are not working correctly with batch size>1 #2904

Closed
shekhovt opened this issue May 13, 2024 · 5 comments · Fixed by #2914
Closed

find_essential and run_5point are not working correctly with batch size>1 #2904

shekhovt opened this issue May 13, 2024 · 5 comments · Fixed by #2914
Labels
help wanted Extra attention is needed

Comments

@shekhovt
Copy link

Describe the bug

The problem is with run_5point solver, which has several filters:

singular_filter = torch.linalg.matrix_rank(coeffs[:, :, :10]) >= torch.max(



All of which can skip processing a batch index without appending anything to the output list E_models. The correspondence between index in this list and the index in the batch is lost.
In the result the shape of the returned tensor will be less than 10*B and it is not possible to determine for which batch indices there were no solution.

The solver is used in find_essential.

Reproduction steps

Run run_5point with 
points1: (B, 5, 2)
points2: (B, 5, 2)
with a large B, e.g. B=100.

Although, the doc string says N>=8 points. Is that necessary?

Expected behavior

Expecting output of shape
(B*10, 3,3)
Although the doctoring says "computed essential matrix with shape :math:(B, 3, 3).", the code returns 10 solutions per batch index when not skipping.

Environment

kornia version 0.7.2

Additional context

No response

@shekhovt shekhovt added the help wanted Extra attention is needed label May 13, 2024
@edgarriba
Copy link
Member

@shekhovt thanks for this finding ! What do you suggest, to add an empty tensor when we skip ? Are you open to fix and ideally expand the tests with this corner case ? /cc @weitong8591 as originally implemented this :)

@shekhovt
Copy link
Author

In the 'continue' cases it can be fixed by
E_models.append(torch.eye(3, device=cs.device, dtype=cs.dtype).repeat(10, 1).reshape(-1, 9))
i.e. writing 10 identity matrices for that image pair. This is consistent with how the variable number of solutions is handled in this function later on.
I am not sure how to fix the singular_filter and not to break anything.

For the tests, if you have a correctness test of batch_size = 1, then it is easy to test also batch_size > 1, right?

@weitong8591
Copy link
Contributor

Hi, thanks for pointing out the missing batch indices (shape difference) due to the filters, i will soon have time to fix it. @edgarriba . probably as you said @shekhovt, to return identity matrices when it fails to make sure to return results for all batches.

@jytime
Copy link

jytime commented May 15, 2024

Hi @shekhovt ,

I encountered this problem as well and made modifications to the code, which now works stably for my training with a batch size of 256. You can find my implementation in the codebase for vggsfm:

https://github.com/facebookresearch/vggsfm/blob/f490bc11b3eb68c1aa832fcd2e230ffacbf575fd/vggsfm/two_view_geo/essential.py#L171

I addressed the singular_filter issue using masking operations. Essential matrices are initialized with eye() and only those with valid singular_filter are replaced by the computed essential matrices. Additionally, I removed the for loop inside the run_5point function (shown below) and replaced it with fully matrix operations, speeding up our training by approximately 12 times.

for bi in range(A.shape[0]):

You could have a try on this implementation if in a hurry. Tong @weitong8591 will review this implementation and merge it to kornia soon.

@shekhovt
Copy link
Author

Hi, thanks a lot! Good job with batching all the components, would be good to merge that to kornia. And an excellent paper too! I was using poselib in the meantime. I will try some of your primitives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants