Fix weird behavior of MaxPointCountDataPointsFilter #464
Conversation
Use C++ random library to generate random indexes Use the more efficient swapCols method from DataPoints's class to swap j and index
|
Can one of the admins verify this patch? |
…e to match the one in the code.
| cloud.times.col(j) = cloud.times.col(idx); | ||
| cloud.times.col(idx) = time; | ||
| } | ||
| std::uniform_int_distribution<size_t> distribution(j, N); |
There was a problem hiding this comment.
How expensive is this operation?
| std::srand(seed); | ||
|
|
||
| for(size_t j=0; j<maxCount; ++j) | ||
| std::default_random_engine randomNumberGenerator(seed); |
There was a problem hiding this comment.
What is the default random engine? Did you evaluate it against other options?
There was a problem hiding this comment.
In the documentation, it says that the default_random_engine is implementation-defined and on my computer (gcc 7.5.0) it says that it's std::minstd_rand0.
I did some benchmark to evaluate the different ways random numbers can be generated:
From that, we can clearly see that only modifying the old algorithm to use the C++11 random library is faster than using C rand function, and it also produce better results at shuffling DataPoints.
| const size_t index = distribution(randomNumberGenerator); | ||
|
|
||
| //Switch columns j and index | ||
| cloud.swapCols(j, index); |
There was a problem hiding this comment.
Can it happen that you swap a point that has already been swapped?
There was a problem hiding this comment.
I think this is a possibility, but I don't know if it can be a problem. To avoid this, we would need keep track of the points that has been already swapped. Do you think it can be a problem?
Change the random number generator to std::minstd_rand instead of default_random_generator Change the location of the uniform distribution outside the for loop Change from uniform_int_distribution<size_t> for uniform_real_distribution<float>
|
Don't hesitate to open new issues if needed. I'll assume that this PR is ready for merge. |
fixes #463
Like mentioned in the issues #463, this is the fix that I'm proposing in order to get better and more consistent results with the filter.
Here's a video showing the results:
Fixed results video