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

Implement SIFT feature detector and descriptor #5472

Merged
merged 128 commits into from Nov 16, 2021

Conversation

faberno
Copy link
Contributor

@faberno faberno commented Jul 14, 2021

Description

Fixes #4499
This PR implements the SIFT feature detection and its descriptor. It was originally developed by David Lowe (https://doi.org/10.1109/ICCV.1999.790410), but this implementation is is mostly based on the paper "Anatomy of the SIFT Method" and its demo by Rey Otero (https://doi.org/10.5201/ipol.2014.82).
I tried to mostly copy the style of ORB with the three Methods: detect, extract, detect_and_extract.
Basic tests and an example in ./doc/examples are provided.

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.

Copy link
Contributor

@grlee77 grlee77 left a comment

Choose a reason for hiding this comment

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

I think this is in good shape.

We probably do want a commit to remove _oversample_bilin, but we can preserve it in the commit history. Lately, we typically squash all commits, but this seems like a case where it could be worth doing a manual rebase to squash a lot of the smaller ones, but preserve some important changes/revisions. I am happy to help with that rebase once we have another approval here.

@grlee77 grlee77 added action: mrg+1 ⏩ type: Enhancement Improve existing features labels Sep 3, 2021
Copy link
Member

@rfezzani rfezzani left a comment

Choose a reason for hiding this comment

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

I added some suggestions, I will come with another round of review soon 😉. Thank you again @faberno

skimage/feature/_sift.pyx Show resolved Hide resolved
skimage/feature/_sift.pyx Show resolved Hide resolved
skimage/feature/_sift.pyx Outdated Show resolved Hide resolved
skimage/feature/_sift.pyx Show resolved Hide resolved
skimage/feature/_sift.pyx Show resolved Hide resolved
skimage/feature/_sift.pyx Show resolved Hide resolved
skimage/feature/_sift.pyx Show resolved Hide resolved
skimage/feature/_sift.pyx Show resolved Hide resolved
skimage/feature/_sift.pyx Outdated Show resolved Hide resolved
skimage/feature/_sift.pyx Show resolved Hide resolved
skimage/feature/_sift.pyx Outdated Show resolved Hide resolved
skimage/feature/_sift.pyx Outdated Show resolved Hide resolved
@rfezzani
Copy link
Member

rfezzani commented Sep 8, 2021

Fixes #4499

Copy link
Member

@stefanv stefanv left a comment

Choose a reason for hiding this comment

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

An impressive piece of work, @faberno, thank you! It is exciting to see SIFT in skimage after all these years.

skimage/feature/sift.py Outdated Show resolved Hide resolved

This example demonstrates the SIFT feature detection and its description
algorithm.

Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to have some context here about what SIFT is, how it is used, and what other feature detectors exist in skimage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay, I was quite busy :D How about:

This example demonstrates the SIFT feature detection and its description
algorithm.

Even though it was already published in 1999, SIFT is still one of the most
popular feature detectors available, as its promises to be "invariant to image
scaling, translation, and rotation, and partially in-variant to illumination
changes and affine or 3D projection" [1]. Its biggest drawback is its runtime,
that's said to be "at two orders of magnitude" [2] slower than ORB, which makes
it unsuitable for real-time applications.

.. [1] D.G. Lowe. "Object recognition from local scale-invariant
features", Proceedings of the Seventh IEEE International
Conference on Computer Vision, 1999, vol.2, pp. 1150-1157.
:DOI:10.1109/ICCV.1999.790410

.. [2] Ethan Rublee, Vincent Rabaud, Kurt Konolige and Gary Bradski
"ORB: An efficient alternative to SIFT and SURF"
http://www.vision.cs.chubu.ac.jp/CV-R/pdf/Rublee_iccv2011.pdf

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks pretty good to me, but I would modify slightly to spell out SIFT early on:

"The scale-invariant feature transform (SIFT) was published in 1999 and is still one of the most popular..."

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a reference to the wikipedia article could also help for those wanting a bit more detail in a less technical format than the conference publications:
https://en.wikipedia.org/wiki/Scale-invariant_feature_transform

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that the link to the paper "ORB: An efficient alternative to SIFT and SURF" does not work anymore. I hope it's alright, that I changed it. Should we also change it in orb.py?

skimage/feature/sift.py Outdated Show resolved Hide resolved
@grlee77 grlee77 mentioned this pull request Sep 17, 2021
return deltas

def _set_number_of_octaves(self, image_shape):
size_min = 12 # minimum size of last octave
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't size_min be a parameter here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that too. In the original implementation this is a constant and can't be changed. My reason not to do it was that the user can still practically increase size_min by lowering n_octaves and I'm not sure if it makes sense to lower size_min even more.
But if you prefer it as a parameter I have no problem with that. We just need to find some minimal value, because size_min too low would probably start to break some things.

Copy link
Member

Choose a reason for hiding this comment

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

An other option would be to make it private attribute of the SIFT class to make it twickable for "expert" users...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds good, I will add it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I plan to merge this PR soon for inclusion in the 0.19 release. If this does not get addressed before then, it can go in a follow-up PR as it would not be a public API change.

Copy link
Member

Choose a reason for hiding this comment

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

Let's leave it as is for now. We can always add this feature if it is requested, but if it is not a parameter in the original it seems unlikely. Also, I am not enthusiastic about private attributes tweakable by expert users: who should either have an API for it, or it should not be adjustable.

@grlee77 grlee77 added this to the 0.19 milestone Nov 15, 2021
@grlee77 grlee77 merged commit fa43626 into scikit-image:main Nov 16, 2021
@grlee77
Copy link
Contributor

grlee77 commented Nov 16, 2021

Okay, this is now in. Thank you, @faberno for the large amount of work and your patience seeing this through!

@faberno
Copy link
Contributor Author

faberno commented Nov 16, 2021

Thanks for all of your support! It was a lot of fun and in the future I'll be happy to contribute again.

@stefanv
Copy link
Member

stefanv commented Nov 16, 2021

It is fantastic to see SIFT finally implemented in scikit-image. Thanks very much @faberno for this great contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⏩ type: Enhancement Improve existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SIFT patent expired
7 participants