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

Feature/speedup random sampling filter #396

Merged

Conversation

YoshuaNava
Copy link
Contributor

@YoshuaNava YoshuaNava commented Aug 5, 2020

In order to resolve issue #393 I worked on speeding up the RandomSamplingDatapointsFilter. For that I implemented two random sampling methods:

  1. Direct RNG, which works similar to the previous implementation, directly querying a random number engine (int), which is transformed to a float in the range 0-1. Result: ~1.5-2x speedup
  2. Uniform, which queries a uniform distribution between 0 and 1. Result: ~30% speedup.

I chose to sample directly a set of floats instead of templating that type. The reason being that float arithmetic should be enough, in my view, to achieve the goal of the filter. Double precision numbers might introduce an overhead.

I refactored a bit the existing unit testing code and ran it locally.

Last but not least, I tried to use the new std::random library, as recommended here. Related talk.

I'm submitting this PR for early feedback. Nonetheless, I still plan to run and plot the samples to get a grasp on the quality of the sampling process, as requested by @pomerlef in the related issue 🙂

@ethzasl-jenkins
Copy link

Can one of the admins verify this patch?

@pomerlef
Copy link
Collaborator

pomerlef commented Aug 5, 2020

add to whitelist

@YoshuaNava
Copy link
Contributor Author

YoshuaNava commented Aug 6, 2020

The build is failing. Do you know what might be the reason?

@pomerlef
Copy link
Collaborator

pomerlef commented Aug 6, 2020

It's the macos built that failed:

/Users/macmini-admin/workspace/pointmatcher/label/osx/pointmatcher/DataPointsFilters/RandomSampling.cpp:91:14: error: no type named 'Index' in namespace 'Eigen'
        for (Eigen::Index i = 0; i < nbPointsIn && j<=nbPointsOut; i++)
             ~~~~~~~^
1 error generated.

make[2]: *** [CMakeFiles/pointmatcher.dir/pointmatcher/DataPointsFilters/RandomSampling.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
2 warnings generated.

make[1]: *** [CMakeFiles/pointmatcher.dir/all] Error 2

make: *** [all] Error 2

@YoshuaNava
Copy link
Contributor Author

YoshuaNava commented Feb 26, 2021

Sorry for the wait, I had switched to other tasks at work.

I modified the file filterProfiler.cpp in the examples folder so that I could run some tests of the random sampling filter. I also added code to the filter to export the random indices matrix to a file. I used the file car_cloud400.csv for the tests (24989 points).

I added a buffer for the random numbers given by the original method, and plotted them:

original

This is what we get when we use the 'Direct RNG' sampling method:

direct_rng

And this is what we get when we enforce uniform sampling:

uniform

All methods seem to give equivalent results (at least visually). I also checked how we can test for the uniformity of a distribution, there's a method from scikit, but I'll wait for input on whether we should test that first.

@pomerlef
Copy link
Collaborator

Looking good!

@pomerlef
Copy link
Collaborator

most probably the same compilation error

pointmatcher/DataPointsFilters/RandomSampling.cpp Outdated Show resolved Hide resolved
pointmatcher/DataPointsFilters/RandomSampling.cpp Outdated Show resolved Hide resolved
pointmatcher/DataPointsFilters/RandomSampling.cpp Outdated Show resolved Hide resolved
pointmatcher/DataPointsFilters/RandomSampling.cpp Outdated Show resolved Hide resolved
pointmatcher/DataPointsFilters/RandomSampling.cpp Outdated Show resolved Hide resolved
pointmatcher/DataPointsFilters/RandomSampling.cpp Outdated Show resolved Hide resolved
@YoshuaNava
Copy link
Contributor Author

I'll rebase berfore we can merge, to keep the history short.

@aguenette aguenette linked an issue Feb 26, 2021 that may be closed by this pull request
@pomerlef pomerlef merged commit 7eaf831 into norlab-ulaval:master Feb 26, 2021
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.

Hotspots in Random Sampling Filter
3 participants