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

Inconsistent handling of default seed in NormalGenerator & UniformGenerator #2985

Open
SunBlack opened this issue Apr 9, 2019 · 3 comments
Labels
effort: low Rough estimate of time needed to fix/implement/solve good first issue Skills/areas of expertise needed to tackle the issue kind: todo Type of issue needs: feedback Specify why not closed/merged yet

Comments

@SunBlack
Copy link
Contributor

SunBlack commented Apr 9, 2019

As already discussed here default seed in random.h is not handled consistently.

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.

Furthermore you get currently following warning:

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

So we should discuss how to adjust code there.

@SunBlack
Copy link
Contributor Author

SunBlack commented Apr 9, 2019

My suggestion: Remove/Deprecate struct Parameters and add new methods:

Instead of setParameters (T min, T max, pcl::uint32_t seed = -1); we should have setParameters (T min, T max); and setParameters (T min, T max, pcl::uint32_t seed); So instead of a default value we use two different methods. First use existing random generator, during second implementation is using first implementation, but set a new seed on random generator.

@stale
Copy link

stale bot commented Jun 6, 2020

Marking this as stale due to 30 days of inactivity. It will be closed in 7 days if no further activity occurs.

@stale stale bot added the status: stale label Jun 6, 2020
@kunaltyagi kunaltyagi added the needs: feedback Specify why not closed/merged yet label Jun 7, 2020
@stale stale bot removed the status: stale label Jun 7, 2020
@kunaltyagi
Copy link
Member

kunaltyagi commented Jun 7, 2020

TODO list:

  • move Parameters to private with name ParameterImpl
  • add an alias and deprecate it
  • deprecate the setParameters function using Parameters
  • add a function without default seed and remove the default value for seed in current impl

This will still break API in that way, but with deprecation.

Any counter opinions (or any other opinions 😄)?

@kunaltyagi kunaltyagi added kind: todo Type of issue effort: low Rough estimate of time needed to fix/implement/solve good first issue Skills/areas of expertise needed to tackle the issue labels Jun 7, 2020
@kunaltyagi kunaltyagi modified the milestones: pcl-1.12.0, pcl-1.13.0 Jun 30, 2021
@mvieth mvieth modified the milestones: pcl-1.13.0, pcl-1.14.0 Nov 14, 2022
@larshg larshg removed this from the pcl-1.14.0 milestone Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: low Rough estimate of time needed to fix/implement/solve good first issue Skills/areas of expertise needed to tackle the issue kind: todo Type of issue needs: feedback Specify why not closed/merged yet
Projects
None yet
Development

No branches or pull requests

4 participants