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.

Sign up for GitHub

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

Jump to bottom

Speed up differentiable 5PC and fix the batch size issue #2914

Merged
merged 12 commits into from
May 22, 2024
Merged

Speed up differentiable 5PC and fix the batch size issue #2914

merged 12 commits into from
May 22, 2024

Conversation

weitong8591
Copy link
Contributor

Changes

  • remove one 'for' loop to speed up the solver, referred to vggsfm.
  • make sure the batch size of the returned E matrices the same as the input by initializing the E matrices with identity matrices. Only those with valid 'singular_filter' are replaced by the computed essential matrices. @jytime
  • add large batch sizes of data in the tests

Fixes #2904

Type of change

  • 📚 Documentation Update
  • 🧪 Tests Cases
  • 🚨 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📝 This change requires a documentation update

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Did you update CHANGELOG in case of a major change?

@weitong8591
remove one for loop to speed up the 5PC alg., and fix the issue of th…
d72f468 
…e inconsistent batch sizes between the input and returned E matrices, referred to [wang2023vggsfm]
@weitong8591
remove one for loop to speed up the 5PC alg., and fix the issue of th…
06814d2 
…e inconsistent batch sizes, referred to [wang2023vggsfm]
@weitong8591
typo
43092d5
@weitong8591
typo
1bbd27a
@weitong8591 weitong8591 marked this pull request as draft May 19, 2024 23:14
@weitong8591 weitong8591 marked this pull request as ready for review May 20, 2024 10:35
edgarriba
edgarriba reviewed May 21, 2024
View reviewed changes
Copy link
Member

@edgarriba edgarriba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any way we could add a specific unit test to raises this corner case ?

@weitong8591
Copy link
Contributor Author

is there any way we could add a specific unit test to raises this corner case ?

Hi, that's a good idea, I found one bug when checking the corner case of degenerate points. I am adding this unit test. Also, to be consistent with 7PC to return all 10 possible solutions, I am removing the code of selecting one out of ten solutions by Sampson distances. will commit soon

edgarriba
edgarriba approved these changes May 22, 2024
View reviewed changes
tests/geometry/epipolar/test_essential.py Outdated Show resolved Hide resolved
@weitong8591 @edgarriba
Update tests/geometry/epipolar/test_essential.py
f7baeaf 
typo

Co-authored-by: Edgar Riba <edgar.riba@gmail.com>
@edgarriba edgarriba merged commit 2606afb into kornia:main May 22, 2024
27 checks passed
@weitong8591
Copy link
Contributor Author

Thanks all for reviewing it!

@weitong8591 weitong8591 deleted the diff_ransac branch May 27, 2024 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edgarriba edgarriba edgarriba approved these changes

@johnnv1 johnnv1 johnnv1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

find_essential and run_5point are not working correctly with batch size>1
3 participants
@weitong8591 @edgarriba @johnnv1