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

Dense matching based line matcher #87

Draft
wants to merge 30 commits into
base: main
Choose a base branch
from
Draft

Conversation

B1ueber2y
Copy link
Member

@B1ueber2y B1ueber2y commented Oct 19, 2024

Added support for RoMa inspired by its keypoint matcher here: https://github.com/Parskatt/RoMa/blob/main/romatch/models/matcher.py#L576. Currently I rely on this PR: Parskatt/RoMa#73 so one needs to RoMa from that branch. @Parskatt

For now I just support a simple one-way nearest neighbor in the end. Ideally we could do one-to-many in the end of the matching. Attached are the results after warping the lines from DeepLSD:

image
image

Added a test script here: https://github.com/cvg/limap/blob/features/dense_matching/scripts/test_matching.py

More feedbacks and tests are very welcome here.

@Parskatt
Copy link

Merging the PR in roma main, just gotta check no regressions first.

Copy link
Collaborator

@MarkYu98 MarkYu98 left a comment

Choose a reason for hiding this comment

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

Able to run the test in a fresh install. Would be better to add dependency to RoMa, and maybe change the path in the testing script to command line args?

scripts/test_matching.py Outdated Show resolved Hide resolved
@@ -0,0 +1,30 @@
import os

import romatch
Copy link
Collaborator

Choose a reason for hiding this comment

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

romatch is not included in requirements.txt or dependencies

Copy link
Member Author

Choose a reason for hiding this comment

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

How about we making a separate installation, as we now already make HAWP, LBD separately installed to reduce dependencies.

@B1ueber2y B1ueber2y marked this pull request as draft October 24, 2024 13:33
requirements.txt Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants