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

added flann based feature matching #33

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

tejus-gupta
Copy link
Contributor

@tejus-gupta tejus-gupta commented Apr 6, 2017

The current match_keypoint method does a brute force feature matching. I have added a function match_keypoint_flann that uses this wrapper for FLANN(Fast Library for Approximate Nearest Neighbors).

This function provides about 30x speedup compared to current brute force matcher(0.27 sec vs 8 sec average time for 4000 features in each image).

@timholy
Copy link
Member

timholy commented Apr 10, 2017

I like the idea very much. If nothing else, FLANN needs to be added to the REQUIRE file (see the errors in the CI testing). Is this going to introduce cross-platform compatibility problems, though? If so, would NearestNeighbors.jl be an alternative? Note that I haven't done any benchmarking on these two packages, they may have wildly different performance characteristics.

For these cases where we dispatch out to external libraries, I wonder if the interface based on ComputationalResources, similar to ImageFiltering, might be more flexible. In particular it might allow several different options and not break this package for users who can't (or don't want to) install FLANN.

@tejus-gupta
Copy link
Contributor Author

tejus-gupta commented Apr 12, 2017

I did some benchmarking for NearestNeighbors.jl. For the same algorithm (KDTree), FLANN has about 10x better performance. I think this is because FLANN has a system for automatically choosing the optimal parameters depending on the dataset.

@timholy
Copy link
Member

timholy commented Apr 12, 2017

OK, but you'll have to resolve those CI errors (click on the Details links abovebelow). I'm especially concerned about AppVeyor since it looks like FLANN.jl doesn't (yet) support Windows.

@timholy
Copy link
Member

timholy commented Apr 12, 2017

There's also https://github.com/KristofferC/NearestNeighbors.jl (I've never tested it).

@tejus-gupta
Copy link
Contributor Author

tejus-gupta commented Apr 12, 2017

I meant to say that I tested KristofferC/NearestNeighbors.jl and it was 10x slower that FLANN.jl.

https://github.com/johnmyleswhite/NearestNeighbors.jl only supports brute force search right now. The README says that they plan to add BallTree and KDTree soon.

@timholy
Copy link
Member

timholy commented Apr 12, 2017

Didn't realize you'd checked the more complete-looking package. In any event, I'm totally prepared to believe that the performance of FLANN is better; it's clearly a huge library focused on performance, and it's widely used.

So I'm completely in favor of this...but the bottom line is that this can't be merged until you ensure that our CI passes. You'll probably need to learn about BinDeps and fix the build script for FLANN.jl so that it installs successfully on all the platforms (that repo also needs to have AppVeyor testing turned on).

@tejus-gupta
Copy link
Contributor Author

tejus-gupta commented Apr 13, 2017

Understood. I am trying to fix the build scripts for FLANN.jl.

@tejus-gupta
Copy link
Contributor Author

Support for 64-bit windows has been added to FLANN.jl - wildart/FLANN.jl#3.
Would it be ok to use NearestNeighbors.jl for windows 32 bit and use the more optimized FLANN.jl for other platforms?

@timholy
Copy link
Member

timholy commented Aug 27, 2017

Absolutely. Basically as long as we don't knowingly break the package, I'd be thrilled with any performance improvement.

@tejus-gupta
Copy link
Contributor Author

@timholy This is ready for review.

Comparison of current and previous matcher -

using ImageFeatures, Images, NearestNeighbors

img1 = rand(Gray{N0f8}, 100, 100);
img2 = rand(Gray{N0f8}, 100, 100);

keypoints_1 = Keypoints(fastcorners(img1, 12, 0.4));
keypoints_2 = Keypoints(fastcorners(img2, 12, 0.4));

brief_params = BRIEF(size = 256, window = 10, seed = 123);

desc_1, ret_keypoints_1 = create_descriptor(img1, keypoints_1, brief_params);
desc_2, ret_keypoints_2 = create_descriptor(img2, keypoints_2, brief_params);

@time match_keypoints(ret_keypoints_1, ret_keypoints_2, desc_1, desc_2, 0.1);
# Average time = 0.051 seconds

@time match_keypoints_bf(ret_keypoints_1, ret_keypoints_2, desc_1, desc_2, 0.1);
# For previous brute force matcher, average time = 0.41 seconds

@tejus-gupta
Copy link
Contributor Author

tejus-gupta commented Jan 2, 2018

Since flann gives approximate results, the test case on line 165 of brief.jl fails. 2 out of the 267 matches returned are incorrect.

@test sum(m[1] + CartesianIndex(100, 200) == m[2] for m in matches) + 1 == length(matches)

v0.0.5 is the last tagged version of FLANN.jl on Metadata.jl. Can we ask the package manager to pull code from v0.1.0 on github for FLANN.jl ?

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.

2 participants