Use random generator from C++11 instead of from Boost #2956

SunBlack · 2019-03-29T17:57:17Z

There are still three occurences of Boost random generator: #2954 (metslib) #2955 (OutofcoreOctreeDiskContainer), #2988 (RangeLikelihood)

Solves partially #2803

SunBlack · 2019-03-29T21:21:57Z

common/include/pcl/common/impl/random.hpp

 /////////////////////////////////////////////////////////////////////////////////////////////////////////
 template <typename T>
 pcl::common::UniformGenerator<T>::UniformGenerator(T min, T max, pcl::uint32_t seed)
  : distribution_ (min, max)
-  , generator_ (rng_, distribution_)
 {
  parameters_ = Parameters (min, max, seed);
  if(parameters_.seed != -1)


Just saw a compiler warning related to this:

common/include/pcl/common/impl/random.hpp:49:23: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] if(parameters_.seed != -1)

Even I didn't touched here anything: Should we fix this warning in this PR (and how?) or in a separate?

We can fix it here. Just static_cast<pcl::uint32_t>(-1).

Just wondering about this:

pcl/common/include/pcl/common/random.h

Lines 163 to 186 in b4c785b

struct Parameters

{

Parameters (T _mean = 0, T _sigma = 1, pcl::uint32_t _seed = 1)

: mean (_mean)

, sigma (_sigma)

, seed (_seed)

{}

T mean;

T sigma;

pcl::uint32_t seed;

};

/** Constructor

* \param[in] mean normal mean

* \param[in] sigma normal variation

* \param[in] seed seeding value

*/

NormalGenerator(T mean = 0, T sigma = 1, pcl::uint32_t seed = -1);

/** Constructor

* \param parameters normal distribution parameters and seed

*/

NormalGenerator(const Parameters& parameters);

One constructor of NormalGenerator has as default seed -1 during the other one (via Parameters) has 1 as default seed.

I think -1 should be the default everywhere. However, thinking more about this, wouldn't it be better if -1 meant "seed with random device" instead of "not seed at all".

I think -1 should be the default everywhere. However, thinking more about this, wouldn't it be better if -1 meant "seed with random device" instead of "not seed at all".

It's a valid point, but I'm not sure if it's a good idea to change the behavior already on this PR. Once the review is complete, I would be open to give it a try on a separate commit or PR just to see if the change is harmless or if it causes unit tests to start exploding like last time... Then we can decide based on the result.

Edit: I started the review before Heiko replied and wrote the comment then. Just leaving it for the extra info on my stance. I'm supportive of addressing it on a separate PR.

Moved discussion to #2985. I think it is better, if this behavior changes are not part of this PR.

SunBlack · 2019-04-01T16:01:04Z

Tests seems to fail, because std random number generator produce other random numbers as boost random number generator with same seed. But not sure if this is always the reason - I even can't reproduce all test failures (Ubuntu just has 3, during Mac has 5). So it would be nice, if another can check this too.

SergioRAgostinho · 2019-04-03T09:38:41Z

Seems like you opened another can of worms :) This is a very big PR, so I'm in favor of splitting it up after we agree on good splitting points. After quickly skimming through the PR and getting acquainted with the new semantics behind <random> the use of std::random_device caught my eye.

One source [1] claims the method might actually throw on some implementations, source [2] only focuses on the aspect that it might still be pseudo random if there's no hardware support. I don't have experience with other platforms other than desktops so I'm not sure if the concern in [1] is applicable to us. I'm tempted to say that no. Opinions?

Regarding the PR split, I feel the per module approach is worthy once more. I'm more interested in revising the core modules first and then progressing towards apps, examples and tools.

[1] - http://www.cplusplus.com/reference/random/random_device/
[2] - https://en.cppreference.com/w/cpp/numeric/random/random_device

SunBlack · 2019-04-03T12:37:24Z

Seems like you opened another can of worms :)

Indeed, I wasn't expected this issues at the time I started :(

This is a very big PR, so I'm in favor of splitting it up after we agree on good splitting points.

I was already thinking about it. My idea was to split everthing out, which causes tests to fail.

One source [1] claims the method might actually throw on some implementations, source [2] only focuses on the aspect that it might still be pseudo random if there's no hardware support. I don't have experience with other platforms other than desktops so I'm not sure if the concern in [1] is applicable to us. I'm tempted to say that no. Opinions?

Well, I don't think [1] is an issue for us. I believe all modern platforms (mobile and desktop) can handle this. Maybe some specific platforms doesn't have it, but I'm nearly sure they are not part of the target group of the PCL.

Regarding the PR split, I feel the per module approach is worthy once more. I'm more interested in revising the core modules first and then progressing towards apps, examples and tools.

Adjusting app, example and tools is easy, because they have no tests which depends on the result of generated values by the random generator, so they are straight forward during core components could be difficult to adjust.

Nevertheless we should discuss about another thing: Is it really necessary to generate random data within core classes? Usually you have separate factory classes in case you need random content. And btw: It is really ugly to have test which expect specific values of random generator.

taketwo · 2019-04-04T07:07:57Z

It is really ugly to have test which expect specific values of random generator.

Fully agree. We can drop such tests, but then we may be left without any tests at all :D

SergioRAgostinho · 2019-04-04T08:06:43Z

Adjusting app, example and tools is easy, because they have no tests which depends on the result of generated values by the random generator, so they are straight forward during core components could be difficult to adjust.

Let's go with the easy cases then first 👍 and progress to the more challenging problems as we progress.

Is it really necessary to generate random data within core classes?

Likely not. But correcting this behaviour is not really mandatory at the moment.

It is really ugly to have test which expect specific values of random generator.

Fully agree. We can drop such tests, but then we may be left without any tests at all :D

😅

...d_rec_framework/include/pcl/apps/3d_rec_framework/pipeline/impl/global_nn_recognizer_crh.hpp

SergioRAgostinho · 2019-04-08T21:19:49Z

common/include/pcl/common/impl/random.hpp

 /////////////////////////////////////////////////////////////////////////////////////////////////////////
 template <typename T>
 pcl::common::UniformGenerator<T>::UniformGenerator(T min, T max, pcl::uint32_t seed)
  : distribution_ (min, max)
-  , generator_ (rng_, distribution_)
 {
  parameters_ = Parameters (min, max, seed);
  if(parameters_.seed != -1)


I think -1 should be the default everywhere. However, thinking more about this, wouldn't it be better if -1 meant "seed with random device" instead of "not seed at all".

It's a valid point, but I'm not sure if it's a good idea to change the behavior already on this PR. Once the review is complete, I would be open to give it a try on a separate commit or PR just to see if the change is harmless or if it causes unit tests to start exploding like last time... Then we can decide based on the result.

Edit: I started the review before Heiko replied and wrote the comment then. Just leaving it for the extra info on my stance. I'm supportive of addressing it on a separate PR.

common/include/pcl/common/random.h

outofcore/include/pcl/outofcore/impl/octree_disk_container.hpp

simulation/src/range_likelihood.cpp

test/registration/test_correspondence_rejectors.cpp

SergioRAgostinho · 2019-04-09T21:16:32Z

test/search/test_search.cpp

      indices.push_back (idx);

-  boost::variate_generator< boost::mt19937, boost::uniform_int<> > rand_indices(boost::mt19937 (), boost::uniform_int<> (0, static_cast<int> (indices.size ()) - 1));
+  std::uniform_int_distribution<> rand_indices(0, static_cast<int> (indices.size ()) - 1);


I don't see the point behind this static_cast<int>. Just remove it.

indices.size () is a size_t, so we have an unsigned type here. In case size() returns 0 we got with static_cast<int> (indices.size ()) - 1 -1 as value, during with without this cast we got std::numeric_limits<size_t>::max() => out of integer range => 0. Because this is test code it doesn't maybe matters. Just want to say we change behavior with it.

test/surface/test_convex_hull.cpp

SergioRAgostinho · 2019-04-09T21:26:42Z

tracking/include/pcl/tracking/boost.h

-#  pragma GCC system_header 
-#endif
-
-#include <boost/random.hpp>


We might need to keep this file around for deprecation purposes, open for discussion. Since it is a header just emit pragma warning.

tracking/include/pcl/tracking/boost.h

SergioRAgostinho · 2019-04-10T12:27:09Z

tracking/include/pcl/tracking/boost.h

+#  pragma GCC system_header 
+#endif
+
+#include <boost/random.hpp>


Hmmm... I'm starting to notice a shortcoming on my request. I wanted to keep pcl/tracking/boost.h around, so it exists as a file, but I didn't want to include any boost headers inside it. If anyone was using this header for some reason it was just because of boost/random.hpp, then removing this line will still break their code, which means we need to keep it around. :/

SergioRAgostinho · 2019-04-11T08:22:26Z

The test are "legitimately" failing on windows. Shall we pull out/revert the the failing case and associated class to a new PR so that we can address the unit test correction there?

…nPoly

SunBlack · 2019-04-11T09:55:05Z

I reverted changes to this test as I have no idea why they are failing. As we have already splitted other changes to other PRs from this PR because of failing tests, we could do this here too.

SergioRAgostinho · 2019-04-11T16:07:15Z

As we have already splitted other changes to other PRs from this PR because of failing tests, we could do this here too.

Let's definitely do that. That way we can look at it from a finer scope.

SunBlack force-pushed the random_generator branch from 077cda6 to 7125cc1 Compare March 29, 2019 19:24

SunBlack commented Mar 29, 2019

View reviewed changes

SunBlack force-pushed the random_generator branch 3 times, most recently from 77f65ba to 49d051a Compare April 1, 2019 15:59

This was referenced Apr 3, 2019

Use random generator from C++11 instead of from Boost in module features #2962

Merged

Use random generator from C++11 instead of from Boost in module sample_consensus #2963

Open

Use random generator from C++11 instead of from Boost in module filters #2964

Closed

Use random generator from C++11 instead of from Boost

3e5eab2

SunBlack force-pushed the random_generator branch from 49d051a to 3e5eab2 Compare April 3, 2019 15:33

SunBlack mentioned this pull request Apr 9, 2019

Inconsistent handling of default seed in NormalGenerator & UniformGenerator #2985

Open

SergioRAgostinho added changelog: enhancement Meta-information for changelog generation changelog: ABI break Meta-information for changelog generation labels Apr 9, 2019

SergioRAgostinho reviewed Apr 9, 2019

View reviewed changes

Small improvements

11add6b

SergioRAgostinho reviewed Apr 10, 2019

View reviewed changes

Use PCL_PRAGMA_WARNING

c485b87

SergioRAgostinho approved these changes Apr 10, 2019

View reviewed changes

taketwo approved these changes Apr 10, 2019

View reviewed changes

taketwo added the c++14 label Apr 10, 2019

Remove unnecessary semicolon

17d99a2

Revert changes in test CorrespondenceRejectors.CorrespondenceRejectio…

7f1379c

…nPoly

Fix indentation

24c6a03

SergioRAgostinho approved these changes Apr 11, 2019

View reviewed changes

SergioRAgostinho merged commit 8866006 into PointCloudLibrary:master Apr 11, 2019

SunBlack deleted the random_generator branch April 11, 2019 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use random generator from C++11 instead of from Boost #2956

Use random generator from C++11 instead of from Boost #2956

SunBlack commented Mar 29, 2019 •

edited

Loading

SunBlack Mar 29, 2019

taketwo Apr 5, 2019

SunBlack Apr 5, 2019 •

edited

Loading

taketwo Apr 6, 2019

SergioRAgostinho Apr 8, 2019

SunBlack Apr 9, 2019

SunBlack commented Apr 1, 2019

SergioRAgostinho commented Apr 3, 2019

SunBlack commented Apr 3, 2019

taketwo commented Apr 4, 2019

SergioRAgostinho commented Apr 4, 2019

SergioRAgostinho Apr 8, 2019

SergioRAgostinho Apr 9, 2019

SunBlack Apr 10, 2019

SergioRAgostinho Apr 9, 2019

SergioRAgostinho Apr 10, 2019

SergioRAgostinho commented Apr 11, 2019

SunBlack commented Apr 11, 2019

SergioRAgostinho commented Apr 11, 2019

	struct Parameters
	{
	Parameters (T _mean = 0, T _sigma = 1, pcl::uint32_t _seed = 1)
	: mean (_mean)
	, sigma (_sigma)
	, seed (_seed)
	{}

	T mean;
	T sigma;
	pcl::uint32_t seed;
	};

	/** Constructor
	* \param[in] mean normal mean
	* \param[in] sigma normal variation
	* \param[in] seed seeding value
	*/
	NormalGenerator(T mean = 0, T sigma = 1, pcl::uint32_t seed = -1);

	/** Constructor
	* \param parameters normal distribution parameters and seed
	*/
	NormalGenerator(const Parameters& parameters);

Use random generator from C++11 instead of from Boost #2956

Use random generator from C++11 instead of from Boost #2956

Conversation

SunBlack commented Mar 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SunBlack Apr 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SunBlack commented Apr 1, 2019

SergioRAgostinho commented Apr 3, 2019

SunBlack commented Apr 3, 2019

taketwo commented Apr 4, 2019

SergioRAgostinho commented Apr 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioRAgostinho commented Apr 11, 2019

SunBlack commented Apr 11, 2019

SergioRAgostinho commented Apr 11, 2019

SunBlack commented Mar 29, 2019 •

edited

Loading

SunBlack Apr 5, 2019 •

edited

Loading