-
Notifications
You must be signed in to change notification settings - Fork 29
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
base: master
Are you sure you want to change the base?
Conversation
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. |
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. |
OK, but you'll have to resolve those CI errors (click on the Details links |
There's also https://github.com/KristofferC/NearestNeighbors.jl (I've never tested it). |
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. |
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). |
Understood. I am trying to fix the build scripts for FLANN.jl. |
Support for 64-bit windows has been added to FLANN.jl - wildart/FLANN.jl#3. |
Absolutely. Basically as long as we don't knowingly break the package, I'd be thrilled with any performance improvement. |
@timholy This is ready for review. Comparison of current and previous matcher -
|
Since flann gives approximate results, the test case on line 165 of brief.jl fails. 2 out of the 267 matches returned are incorrect.
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 ? |
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).