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.

Sign up for GitHub

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

Jump to bottom

Fix weird behavior of MaxPointCountDataPointsFilter #464

Merged
merged 5 commits into from
Jul 7, 2021
Merged

Fix weird behavior of MaxPointCountDataPointsFilter #464

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bec0f70
Fix weird behavior of MaxPointCountDataPointsFilter
aguenette May 31, 2021
f90a924
Changed the MaxPointCountDataPointsFilter seed parameter default valu…
aguenette Jun 8, 2021
31c0cda
Update MaxPointCount.cpp
aguenette Jul 5, 2021
d78cfcb
Merge branch 'master' into fix/max_point_count_filter
aguenette Jul 6, 2021
fd89b2c
Merge branch 'master' into fix/max_point_count_filter
pomerlef Jul 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions doc/DataFilters.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ __Impact on the number of points:__ reduces number of points

|Parameter |Description |Default value |Allowable range|
|--------- |:---------|:----------------|:--------------|
|seed | srand seed | 0 | min: 0 max: 2147483647 |
|seed | srand seed | 1 | min: 0 max: 2147483647 |
|maxCount |number of points beyond which subsampling occurs | 1000 | min: 0, max: 2147483647|

### Example
Expand Down Expand Up @@ -699,4 +699,3 @@ The number of points in a point cloud can be reduced by taking random point subs
## Where To Go From Here

This concludes the overview of data point filters. For a tutorial on writing a simple application for applying data point filters to an input point cloud, go [here](ApplyingDataFilters.md). To learn more about the general configuration of the ICP chain go [here](DefaultICPConfig.md).

49 changes: 17 additions & 32 deletions pointmatcher/DataPointsFilters/MaxPointCount.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,28 +33,29 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

*/
#include "MaxPointCount.h"
#include <random>

// MaxPointCountDataPointsFilter
// Constructor
template<typename T>
MaxPointCountDataPointsFilter<T>::MaxPointCountDataPointsFilter(const Parameters& params):
PointMatcher<T>::DataPointsFilter("MaxPointCountDataPointsFilter",
PointMatcher<T>::DataPointsFilter("MaxPointCountDataPointsFilter",
MaxPointCountDataPointsFilter::availableParameters(), params),
maxCount(Parametrizable::get<size_t>("maxCount"))
{
try
try
{
seed = this->template get<size_t>("seed");
}
catch (const InvalidParameter&)
}
catch (const InvalidParameter&)
{
seed = static_cast<size_t>(1); // rand default seed number
}
}

// Compute
template<typename T>
typename PointMatcher<T>::DataPoints
typename PointMatcher<T>::DataPoints
MaxPointCountDataPointsFilter<T>::filter(const DataPoints& input)
{
DataPoints output(input);
Expand All @@ -66,35 +67,20 @@ MaxPointCountDataPointsFilter<T>::filter(const DataPoints& input)
template<typename T>
void MaxPointCountDataPointsFilter<T>::inPlaceFilter(DataPoints& cloud)
{
const size_t N = static_cast<size_t> (cloud.features.cols() - 1);
if (maxCount <= N)
const auto N = static_cast<size_t>(cloud.features.cols() - 1);
aguenette marked this conversation as resolved.
Show resolved Hide resolved

if (maxCount <= N)
{
//Re-init seed at each call, to ensure same results
std::srand(seed);

for(size_t j=0; j<maxCount; ++j)
std::default_random_engine randomNumberGenerator(seed);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the default random engine? Did you evaluate it against other options?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

int_vs_real_minstd_rand0

int_vs_real_minstd_rand

real_minimal_standard_vs_mersenne_twister_double

real_minimal_standard_vs_mersenne_twister_float

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.


for(size_t j = 0; j < maxCount; ++j)
aguenette marked this conversation as resolved.
Show resolved Hide resolved
{
//Get a random index in [j; N]
const size_t idx = j + static_cast<size_t>((N-j)*(static_cast<float>(std::rand()/static_cast<float>(RAND_MAX))));

//Switch columns j and idx
const auto feat = cloud.features.col(j);
cloud.features.col(j) = cloud.features.col(idx);
cloud.features.col(idx) = feat;

if (cloud.descriptors.cols() > 0)
{
const auto desc = cloud.descriptors.col(j);
cloud.descriptors.col(j) = cloud.descriptors.col(idx);
cloud.descriptors.col(idx) = desc;
}
if (cloud.times.cols() > 0)
{
const auto time = cloud.times.col(j);
cloud.times.col(j) = cloud.times.col(idx);
cloud.times.col(idx) = time;
}
std::uniform_int_distribution<size_t> distribution(j, N);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How expensive is this operation?

const size_t index = distribution(randomNumberGenerator);

//Switch columns j and index
cloud.swapCols(j, index);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it happen that you swap a point that has already been swapped?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
//Resize the cloud
cloud.conservativeResize(maxCount);
Expand All @@ -103,4 +89,3 @@ void MaxPointCountDataPointsFilter<T>::inPlaceFilter(DataPoints& cloud)

template struct MaxPointCountDataPointsFilter<float>;
template struct MaxPointCountDataPointsFilter<double>;