Skip to content

Using std types instead of boost for Gaussian sampling#2351

Merged
sjahr merged 5 commits intomoveit:mainfrom
Shobuj-Paul:std_gaussian_sampling
Oct 19, 2023
Merged

Using std types instead of boost for Gaussian sampling#2351
sjahr merged 5 commits intomoveit:mainfrom
Shobuj-Paul:std_gaussian_sampling

Conversation

@Shobuj-Paul
Copy link
Copy Markdown
Contributor

@Shobuj-Paul Shobuj-Paul commented Sep 9, 2023

Description

Getting rid of boost for sampling from a gaussian distribution and instead using standard library functions, wherever available.

Fixes #2166

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@Shobuj-Paul Shobuj-Paul force-pushed the std_gaussian_sampling branch 2 times, most recently from a06fcf5 to 05f930f Compare September 10, 2023 09:36
@codecov
Copy link
Copy Markdown

codecov bot commented Sep 10, 2023 •
edited
Loading

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (84d3937) 50.83% compared to head (48fa1ef) 50.34%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2351      +/-   ##
==========================================
- Coverage   50.83%   50.34%   -0.49%     
==========================================
  Files         391      390       -1     
  Lines       32125    31954     -171     
==========================================
- Hits        16328    16084     -244     
- Misses      15797    15870      +73

see 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Shobuj-Paul
Copy link
Copy Markdown
Contributor Author

Shobuj-Paul commented Sep 11, 2023 •
edited
Loading

@henningkayser
There isn't a standard function for boost::variate_generator (refer this link).
Can we think of writing our own implementation of variate_generator, or try to patch the gaussian_ variable together with std::bind?

I suppose using the <tr1/random> header does not read as standard library?

@Shobuj-Paul Shobuj-Paul force-pushed the std_gaussian_sampling branch 4 times, most recently from 963511f to 01da506 Compare September 16, 2023 05:24
@Shobuj-Paul
Copy link
Copy Markdown
Contributor Author

Shobuj-Paul commented Sep 17, 2023

I am directly calling the dist_(eng_) for updating output matrix, becasue that is what () operator is doing in variate_generator also as per my understanding. Please review and let me know if that is correct. Also, how do I test if this is working correctly?

@Shobuj-Paul Shobuj-Paul marked this pull request as ready for review September 17, 2023 09:52
@Shobuj-Paul Shobuj-Paul force-pushed the std_gaussian_sampling branch 4 times, most recently from 756df20 to d9a89f3 Compare September 28, 2023 12:34
@Shobuj-Paul Shobuj-Paul force-pushed the std_gaussian_sampling branch 3 times, most recently from 68df427 to 85e4d96 Compare October 5, 2023 14:19
@Shobuj-Paul Shobuj-Paul force-pushed the std_gaussian_sampling branch from 85e4d96 to e243823 Compare October 10, 2023 17:02
sjahr
sjahr requested changes Oct 11, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@sjahr sjahr left a comment

Choose a reason for hiding this comment

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

@henningkayser Not sure if the rng initialized with the default value has the desired effect.

@Shobuj-Paul Shobuj-Paul force-pushed the std_gaussian_sampling branch from e243823 to 2658e4b Compare October 11, 2023 07:19
@Shobuj-Paul Shobuj-Paul requested a review from sjahr October 11, 2023 10:58
@Shobuj-Paul Shobuj-Paul force-pushed the std_gaussian_sampling branch 3 times, most recently from 4f8c1b8 to 3c738b0 Compare October 19, 2023 06:26
sjahr
sjahr reviewed Oct 19, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@sjahr sjahr left a comment

Choose a reason for hiding this comment

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

Sorry @Shobuj-Paul I just discover an even simpler way to do this. We had a PR ~1 year ago which unified the use of random number generators across the repository(see the changes for the CHOMP planner as references for STOMP) by using a unified implementation to create a random number generator . It is more performant since creating a random device is computationally expensive. Can you test my suggestions? All that's left to do would be to add <depend>rsl</depend> to the CMakeLists.txt. If that works this PR is finally good to go 😅

@Shobuj-Paul
*Changed boost namespace to std
52b96a8 
*Need to compare function implementations
*Find an equivalent implementation for variate_generator
@Shobuj-Paul
calling Distribution(Engine) directly
de59c47
@Shobuj-Paul Shobuj-Paul force-pushed the std_gaussian_sampling branch from 3c738b0 to 1cbb59e Compare October 19, 2023 10:35
@sjahr sjahr merged commit 3ba9d9d into moveit:main Oct 19, 2023
@Shobuj-Paul Shobuj-Paul deleted the std_gaussian_sampling branch October 19, 2023 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@henningkayser henningkayser henningkayser approved these changes

@sjahr sjahr sjahr approved these changes

Assignees

@Shobuj-Paul Shobuj-Paul

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Modernize random sampling in STOMP

3 participants

@Shobuj-Paul @henningkayser @sjahr