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

nanoFLANN implementation for KdTree #84

Closed
wants to merge 1 commit into from

Conversation

fran6co
Copy link
Contributor

@fran6co fran6co commented May 14, 2013

I made a port of https://code.google.com/p/nanoflann/ for PCL.

Not sure how to add the nanoflann library as a dependency, I copied it on the impl folder as it is just a hpp file. Not sure if it's the proper way to do that.

I didn't do any formal benchmark on it still, the author says that it's faster than the original FLANN but I don't see much difference in my code.

My rationale of doing this port was:

  • doesn't need to build libflann, nanoflann it's just a .hpp file
  • no memory leaks, I found several memoryleaks in the libflann that were hard to debug (I may go back to it and try to fix them)
  • the author says it's faster

@fran6co
Copy link
Contributor Author

fran6co commented May 14, 2013

To use this implementation with pcl::search::KdTree #81 needs to be merged. Adding the pre compilation to PCL_INSTANTIATE would be nice, but it's not a requirement (just include pcl/search/impl/kdtree.hpp)

@mariusmuja
Copy link
Contributor

Hi fran6co,

Was is in the latest version of FLANN that you found the memory leaks? If so, could you point them to me, so I can fix them ?

Thanks,
Marius

@andersgb1
Copy link
Contributor

I'm interested in this discussion too. As far as I understood, nanoflann
should be faster due to some compiler optimizations the author
did? Additionally, he mentions that it is only suitable for low-dimensional
point sets, so I guess that it should also be clarified whether this is
even usable for feature matching..?

On 17 May 2013 06:07, Marius Muja notifications@github.com wrote:

Hi fran6co,

Was is in the latest version of FLANN that you found the memory leaks? If
so, could you point them to me, so I can fix them ?

Thanks,
Marius


Reply to this email directly or view it on GitHubhttps://github.com//pull/84#issuecomment-18042838
.

@mariusmuja
Copy link
Contributor

FLANN contains two distinct kd-tree implementations: one is a classic kd-tree that performs exact search and is efficient for low dimensional points and the other is a randomized k-d forest which performs approximate nearest neighbour search and is optimized for high dimensional features. nanoFLANN contains the first one, so it would not be suitable for matching high dimensional features.

@andersgb1
Copy link
Contributor

Yes, my point was more whether this should somehow be signalled in the PCL
wrappers..?

On 17 May 2013 09:33, Marius Muja notifications@github.com wrote:

FLANN contains two distinct kd-tree implementations: one is a classic
kd-tree that performs exact search and is efficient for low dimensional
points and the other is a randomized k-d forest which performs approximate
nearest neighbour search and is optimized for high dimensional features.
nanoFLANN contains the first one, so it would not be suitable for matching
high dimensional features.


Reply to this email directly or view it on GitHubhttps://github.com//pull/84#issuecomment-18047601
.

@fran6co
Copy link
Contributor Author

fran6co commented May 17, 2013

@mariusmuja I don't know where the memory leaks exactly are, I'm using the pcl library without debug symbols. But the XCode inspectors shows some heavy leaking when using KdTreeFLANN that go away when using the KdTreeNanoFLANN. I'll try to get more info about it and post it on the libflann repository.

@aichim
Copy link
Member

aichim commented May 17, 2013

Francisco, thanks a lot for your input! We are really eager to get these fixed!

@jkammerl
Copy link
Member

Hi @fran6co, just a quick note: the performance of the kdtrees should strongly depend on the datasets you test it with. It would be great if you could test it on different sets (kinect, trimble data, etc). You'll find a lot of different dataset here: http://svn.pointclouds.org/data/

@fran6co
Copy link
Contributor Author

fran6co commented Jun 1, 2013

@mariusmuja I found the memory leak but it was fixed by a newer version (I was using 1.8.2) in this commit.
Couldn't find any time to run the benchmarks yet.

@aichim
Copy link
Member

aichim commented Jun 14, 2013

What is the status here? Julius, can you take over and coordinate please?

@fran6co
Copy link
Contributor Author

fran6co commented Jun 17, 2013

@jkammerl I had some spare time and I did run a benchmark. Found this old ticket with some benchmarking code and a sample cloud here.
I ran the benchmark on a MacBook Air 13' 1.8GHz i5, 8Gb of RAM and this are the results:

Cloud size: 599666

Building FLANN kdtree using PCL wrapper
Building took: 0.309649
Searching
Searching took: 114.506
neighbors/query: 2108.1

Building nanoFLANN kdtree using PCL wrapper
Building took: 0.411368
Searching
Searching took: 145.148
neighbors/query: 2108.1

The results are very similar, but I'm not sure if I'm configuring nanoflann the correct way. One other thing that surprised me is that the original benchmark in 2011 for the PCL wrapper was 0.58 for building and 9.6 for searching. Now there is one digit difference! What hardware did you use @mariusmuja ?

@andersgb1
Copy link
Contributor

I think 35|25 % increases in build|search times is quite drastic! According to the author of nanoflann, there should have been up to 50 % decreases in build|search times for a 3D data set of this size. Did FLANN just get a whole lot better over the last couple of versions?

@fran6co
Copy link
Contributor Author

fran6co commented Jun 17, 2013

I'm sure it's something I did wrong. The nanoflann library recommends to create an adapter to whatever data structure you use. http://nanoflann.googlecode.com/svn/trunk/examples/pointcloud_adaptor_example.cpp Here is an example for a pointcloud adapter, I wanted to keep simple and I'm using the matrix transformation.
If I have some spare time I'll try to switch to that.

@fran6co
Copy link
Contributor Author

fran6co commented Jun 17, 2013

I recompiled this branch using the cmake configuration and I got this numbers:

Cloud size: 599666

Building FLANN kdtree using PCL wrapper
Building took: 0.323369
Searching
Searching took: 112.544
neighbors/query: 2108.1

Building FLANN kdtree using PCL wrapper
Building took: 0.496915
Searching
Searching took: 128.581
neighbors/query: 2108.1

A bit better, but still not good enough. I think that using the adapter should speed up the building.

@aichim
Copy link
Member

aichim commented Jul 9, 2013

Hi,
@fran6co ,Did you get the chance to try the version with the adapter?

@mariusmuja , do you think it makes sense to pursue this and include nanoflann in PCL? By looking at their website, the improvements seem to be mostly compiler-related, and not necessarily due to different/better algorithms.

@ghost ghost assigned aichim Sep 7, 2013
@fran6co
Copy link
Contributor Author

fran6co commented Sep 13, 2013

@aichim I got around to do a new version with the adapter, the building time improved a lot and it takes around the same time as the normal FLANN kdtree. I don't see the perfomance gains that the nanoflann projects claims, maybe I'm doing something wrong with the optimizations. The major improvement I see is with portability (for example ARM devices wouldn't need to build libflann there).

Cloud size: 599666

Building FLANN kdtree using PCL wrapper
Building took: 0.365408
Searching
Searching took: 112.046
neighbors/query: 2108.1

Building nanoFLANN kdtree using PCL wrapper
Building took: 0.343304
Searching
Searching took: 117.02
neighbors/query: 2108.1

@fran6co
Copy link
Contributor Author

fran6co commented Sep 13, 2013

I had a second look at the nanoFlann page, it seems that the big performance gains come for big pointclouds (10^6 or bigger). I'm using a cloud of size 600k that is just around the size where FLANN and nanoFLANN are the same, I'll check if I can test it with bigger clouds.

@rbrusu
Copy link
Member

rbrusu commented Dec 16, 2013

Is there any follow up to this issue? @aichim @jkammerl

@rbrusu
Copy link
Member

rbrusu commented Jan 13, 2014

@fran6co what should we do with this issue?

@fran6co
Copy link
Contributor Author

fran6co commented Jan 13, 2014

@rbrusu, I think nanoFlann is a good alternative to FLANN for embedded systems or complex dev environments where depencies should be kept at a minimum.
In the other hand I have no longer the need for nanoFlann in my project, if #81 gets merged it's not difficult to implement nanoFlann on a per project basis.

@jkammerl
Copy link
Member

According to the posted performance results, I don't see any significant performance gain using nanoFlann. Also given that flann has an active and supportive community I am not sure if it makes sense to switch to nanoflann.

@jkammerl
Copy link
Member

Great investigation though!

@fran6co
Copy link
Contributor Author

fran6co commented Jan 13, 2014

I didn't try for bigger clouds, I think we are going to see major gains there as the graphs in the nanoflann homepage show. I don't have the time to test it at the moment though.

@rbrusu
Copy link
Member

rbrusu commented Jan 13, 2014

Thanks guys! I'll close the issue for now then.

@rbrusu rbrusu closed this Jan 13, 2014
@tkircher
Copy link

Now that FLANN has been abandoned while nanoflann is actively maintained, it may be worth giving this a second look.

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.

7 participants