[ros2] WIP: Remove random_numbers dependency

[sjahr]

Description

This PR aims to remove the random number dependency from geometric_shapes. To do so, the cpp std library <random> is used directly. Instead of the random_number::RandomNumberGenerator, a simpler one is defined in the header random_number_utils.hpp.

Furthermore, the API of samplePointInside is changed by removing the random number generator argument. I did this to aviod the need for the user to create an extra RandomNumberGenerator instance when using this function

Generating random quaternions can be done with the constructor from Eigen itself. For example like this:

Eigen::Quaterniond rand_quaternion = Eigen::Quaterniond::UnitRandom();

[codecov[bot]]

Codecov Report

Merging #223 (39c5354) into ros2 (8699136) will increase coverage by 0.38%. The diff coverage is 86.05%.

@@            Coverage Diff             @@
##             ros2     #223      +/-   ##
==========================================
+ Coverage   54.60%   54.97%   +0.38%     
==========================================
  Files          11       12       +1     
  Lines        2026     2056      +30     
==========================================
+ Hits         1106     1130      +24     
- Misses        920      926       +6     

Impacted Files	Coverage Δ
include/geometric_shapes/bodies.h	93.55% <ø> (ø)
include/geometric_shapes/random_number_utils.hpp	80.00% <80.00%> (ø)
src/bodies.cpp	71.45% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[sjahr]

Sorry, but I don't yet get the reading behind this PR. IMO this PR achieves the following:

• It replaces boost types with std types, which I definitely support.
• It moves the RNG definition from the RNG package into this package. Why? This reduces modularity.
• It introduces thread-local RNG singletons. Why? One motivation could be found here. But, I would like to see the argumentation for this both in the PR comment and a code comment wink
• It changes public API all over. Is this really needed?

I suggest to stick to the concept of a RandomNumberGenerator implemented in a separate package. Whether this should be a singleton or thread-local, is the user's decision isn't it?

@rhaschke Thank you for this review and please excuse my long response time. The motivation of this PR was to remove the dependency on the random numbers package in order to reduce the number of packages to maintain and to make use of random. This might be at the cost of modularity but I think in this case the advantages outweight and the change of the interface to

  virtual bool samplePointInside(std::function<double(double, double)> random_value, unsigned int max_attempts,

allows the users to use their own random number generation algorithms.

As you pointed out, the reason for the thread-local singleton is on one hand thread-safety and in addition to that, I wanted to make the usage computationally more efficient by motivation the user to minimize the number of constructions of random number generator instances.

I've added more comments to the class definition to document these motivations

[peci1]

I also don't understand the motivation behind this PR.

Removing a dependency by copying its code inside the dependent package is not the way modular systems are built. Moreover, random_numbers is a simple package that only depends on boost.

Apart from the architectural point of view, I also don't have much understanding for the mathematical meaning. You suggest that with this PR users can provide their custom RNG implementations. However, all uses of the RNG in this package expect an RNG that draws from a strictly bounded distribution. I know only one such distribution - the uniform one. Okay, in an extreme case, you could use a "sloped uniform" distribution, but that's weird - do you think if the user wants the distribution to have some slope along the X axis of a box, that he will want to have the same slope along the other two axes? Okay, so why not passing 1-3 lambdas to the samplePointInside functions, one for each axis? I don't like the direction where it's heading. If, on the other hand, you'd pass an unbounded distribution (like Gaussian), the implicit contract of the sampling methods would have been broken and the sampling might take extremely long because there could be much more misses of the shape's volume than expected. I assume if someone needs to sample Gaussians from a box, then it would probably deserve a more clever algorithm than the generic one from this library. Or do you have some concrete example of a non-uniform distribution you'd like to pass to the sampling methods?

[tylerjw]

The boost random generator was created before there was a good implementation of random number generation in the stl. In some ways what is in the stl now resembles the interface in boost in that it is annoyingly complex to use correctly but it does provide industrial strength randomness.

We hear the complaint of copying code into this package going in the wrong direction. We also have the constraint that updating random_numbers itself to be more modern means dealing with all the code that depends on the interfaces of random_numbers that we would break. It seems easier to us to replace random_numbers with a new package we can incrementally adopt. We are working on doing that work here: https://github.com/PickNikRobotics/RSL/blob/main/src/rand.cpp

Once this is ready for use (released) we will adapt this PR to use this new package that contains a random number generator function.

[sjahr]

I also don't understand the motivation behind this PR.

Removing a dependency by copying its code inside the dependent package is not the way modular systems are built. Moreover, random_numbers is a simple package that only depends on boost.

Apart from the architectural point of view, I also don't have much understanding for the mathematical meaning. You suggest that with this PR users can provide their custom RNG implementations. However, all uses of the RNG in this package expect an RNG that draws from a strictly bounded distribution. I know only one such distribution - the uniform one. Okay, in an extreme case, you could use a "sloped uniform" distribution, but that's weird - do you think if the user wants the distribution to have some slope along the X axis of a box, that he will want to have the same slope along the other two axes? Okay, so why not passing 1-3 lambdas to the samplePointInside functions, one for each axis? I don't like the direction where it's heading. If, on the other hand, you'd pass an unbounded distribution (like Gaussian), the implicit contract of the sampling methods would have been broken and the sampling might take extremely long because there could be much more misses of the shape's volume than expected. I assume if someone needs to sample Gaussians from a box, then it would probably deserve a more clever algorithm than the generic one from this library. Or do you have some concrete example of a non-uniform distribution you'd like to pass to the sampling methods?

As Tyler said, due to the feedback we got, I reconsidered and we added a minimal package with the proposed random number generation instead of reducing the whole package to a simple header.

Regrading your second statement I am a bit confused because a using a different random number generator does not necessarily imply that a different distribution is used. The random package contains multiple random number engines (algorithms to create random numbers) that can be used to create a bounded uniform distribution. In addition to that, it might be useful for testing to create a function that creates specific values and to pass it as fake random number generator. From my understanding, this change increases modularity because users don't have to use our generator implementation based on the mersenne twister algorithm. Please correct me if I am missing something here

[peci1]

From my understanding, this change increases modularity because users don't have to use our generator implementation based on the mersenne twister algorithm. Please correct me if I am missing something here

You're right that the generator and distribution are two distinct parts. I just did not see too much value in allowing use of other generators (well, you want a uniform distribution in the end, so you should not generate too much differently than MT). If you'd provide a deterministic fake generator, these sampling functions could even get broken. The only real difference could be usage of a true RNG, but that seems like an overkill to me for this use-case...