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

New feature: SOLD2 line detector and descriptor #1507

Closed
wants to merge 25 commits into from

Conversation

rpautrat
Copy link
Contributor

Changes

Addition of the SOLD2 line detector and descriptor. I separated the detector and descriptor to allow to only detect lines, or detect&describe and perform line matching across images.

The added code does not require any special package outside of numpy and pytorch.

Fixes #1124

Type of change

  • 📚 Documentation Update
  • 🧪 Tests Cases
  • 🔬 New feature (non-breaking change which adds functionality)
  • 📝 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

@ducha-aiki
Copy link
Member

Wow, what a Christmas gift!

@rpautrat
Copy link
Contributor Author

Yes, I finally found some time to do it!
Merry Christmas ;)

@edgarriba
Copy link
Member

@rpautrat awesome and thanks! I'll review over the next week and target to be merged for the release after new year's :)

@edgarriba edgarriba added feature request New feature or request module: feature labels Jan 5, 2022
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.

some suggestions and mostly some functions that need to be re-implemented in pytorch instead of numpy. One general question: how much this method generalizes and how easy/difficult to fine-tune? just to understand how sensible is in front new data. For future after merge, a quick colab showing for example to reproduce that nice gif would be great.

kornia/feature/sold2/sold2.py Outdated Show resolved Hide resolved
kornia/feature/sold2/sold2.py Outdated Show resolved Hide resolved
kornia/feature/sold2/sold2.py Outdated Show resolved Hide resolved
kornia/feature/sold2/sold2.py Outdated Show resolved Hide resolved
kornia/feature/sold2/sold2.py Outdated Show resolved Hide resolved
kornia/feature/sold2/sold2_detector.py Outdated Show resolved Hide resolved
kornia/feature/sold2/sold2_detector.py Outdated Show resolved Hide resolved
kornia/feature/sold2/sold2_detector.py Outdated Show resolved Hide resolved
kornia/feature/sold2/backbones.py Outdated Show resolved Hide resolved
kornia/feature/sold2/backbones.py Show resolved Hide resolved
@edgarriba edgarriba added the 2 Priority 2 😅 Medium priority label Jan 7, 2022
@rpautrat
Copy link
Contributor Author

Thanks for the detailed review and feedback! I might need a bit of time for the re-implementation from numpy to pytorch because there are some deadlines coming soon.

The colab showing how to reproduce the GIF would be feasible, yes.

Regarding generalization and fine-tuning, it generalizes fairly well indoors, but the performance is not as good outdoors. I was planning to release an outdoor model at some point, but it takes some time to train and tune it properly. Fine-tuning is indeed cumbersome unfortunately, because you need to generate the pseudo ground truth on the new dataset each time... But if I manage to get a good outdoor model, it might be less essential to fine-tune it.

@edgarriba
Copy link
Member

Thanks for the detailed review and feedback! I might need a bit of time for the re-implementation from numpy to pytorch because there are some deadlines coming soon.

The colab showing how to reproduce the GIF would be feasible, yes.

Regarding generalization and fine-tuning, it generalizes fairly well indoors, but the performance is not as good outdoors. I was planning to release an outdoor model at some point, but it takes some time to train and tune it properly. Fine-tuning is indeed cumbersome unfortunately, because you need to generate the pseudo ground truth on the new dataset each time... But if I manage to get a good outdoor model, it might be less essential to fine-tune it.

Sounds good ! No rush here from our side. Let us know if you need help with any part.

@edgarriba edgarriba marked this pull request as draft January 30, 2022 17:44
@edgarriba edgarriba added the wip 🛠️ Work in progress label Jan 30, 2022
@rpautrat rpautrat marked this pull request as ready for review March 30, 2022 09:23
@rpautrat
Copy link
Contributor Author

Hi, I had time to work again on this PR.

I have moved all numpy functions to Pytorch as suggested, and opted for the torchvision nms operation. It was the fastest among the kornia one, torchvision one and SuperPoint one.

After consideration, releasing the code generating the GIF in a colab might not be possible, as it was partially based on the SuperPoint demo (https://github.com/magicleap/SuperPointPretrainedNetwork). The license stipulates that you can only use the code for research or non-profit purposes, so releasing it on Colab might be against it...
I can however do a Colab reimplementing the notebook that I had in my official SOLD2 release (https://github.com/cvg/SOLD2/blob/main/notebooks/match_lines.ipynb), but importing SOLD2 from kornia this time.

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.

@ducha-aiki @shijianjian can you review

@rpautrat sounds good about the colab thing -- the example looks really nice

@@ -0,0 +1,436 @@
"""Implements several backbone networks."""
Copy link
Member

Choose a reason for hiding this comment

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

just as a meta-thought: @ducha-aiki @shijianjian we might plan at some point to make a low level-backbones-features API. Open to suggestions

return state_dict


class WunschLineMatcher:
Copy link
Member

Choose a reason for hiding this comment

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

@ducha-aiki we could move this the matching API

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I will take a look after this PR merged

kornia/feature/sold2/sold2.py Outdated Show resolved Hide resolved
kornia/feature/sold2/sold2_detector.py Outdated Show resolved Hide resolved
kornia/feature/sold2/sold2.py Outdated Show resolved Hide resolved
Copy link
Member

@ducha-aiki ducha-aiki left a comment

Choose a reason for hiding this comment

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

Besides the @edgarriba comments about imports, LGTM

rpautrat and others added 4 commits March 31, 2022 15:21
@ducha-aiki ducha-aiki mentioned this pull request Aug 30, 2022
12 tasks
@ducha-aiki
Copy link
Member

@rpautrat I have added some minor fixes, preserving commit history here: #1844
Thank you a lot for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 Priority 2 😅 Medium priority feature request New feature or request module: feature wip 🛠️ Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add SOLD2 line detector/descriptor/matcher to kornia
3 participants