Skip to content

Random interface#3957

Draft
SergioRAgostinho wants to merge 6 commits intoPointCloudLibrary:masterfrom
SergioRAgostinho:random-interface
Draft

Random interface#3957
SergioRAgostinho wants to merge 6 commits intoPointCloudLibrary:masterfrom
SergioRAgostinho:random-interface

Conversation

@SergioRAgostinho
Copy link
Member

An implementation of the discussion in #3724.

Opening up for comments.

Closes #3724.

@SergioRAgostinho SergioRAgostinho added changelog: enhancement Meta-information for changelog generation module: common labels Apr 21, 2020
@shrijitsingh99
Copy link
Contributor

Would this be used as a base for UniformGenerator etc. (#3944)?

@SergioRAgostinho
Copy link
Member Author

SergioRAgostinho commented Apr 22, 2020

Would this be used as a base for UniformGenerator etc. (#3944)?

I gave it a try already and I can make it a base to UniformGenerator while still preserving the current behavior without changes to the test suite. It looks ugly though because of the -1 default seed behavior.

@shrijitsingh99
Copy link
Contributor

I gave it a try already and I can make it a base to UniformGenerator while still preserving the current behavior without changes to the test suite. It looks ugly though because of the -1 default seed behavior.

Can we go ahead and break the current behavior? Since it doesn't seem to be widely used as of now and having this new interface would allow to go head and replace all old and bad practices of random number generation.

@SergioRAgostinho
Copy link
Member Author

Can we go ahead and break the current behavior? Since it doesn't seem to be widely used as of now and having this new interface would allow to go head and replace all old and bad practices of random number generation.

Behavior breakage is for a different PR. Right not I'm just trying to hot-swap a couple of instances to validate how potentially useful this class is.

Copy link
Member

@taketwo taketwo left a comment

Choose a reason for hiding this comment

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

Just throwing in a few naming ideas, feel free to disregard:

  1. use Mixin instead of Base
    add Random to the names of methods, e.g. (setRandomSeed, resetRandomEngine)

The second one is because terms like "seed" and "engine" are overloaded. In our CV context, the former may also mean a starting point in e.g. floodfill segmentation. And in computing in general, the latter may mean different things. (Imagine resetEngine() popping up in an auto-completion list for some PCL class, may look confusing).

}

protected:
SeedT seed_;
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure about making this protected and not private?

@stale
Copy link

stale bot commented Jun 2, 2020

Marking this as stale due to 30 days of inactivity. Commenting or adding a new commit to the pull request will revert this.

@stale stale bot added the status: stale label Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog: enhancement Meta-information for changelog generation module: common status: stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[filters] Create a SamplingFilter base class

4 participants