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

Using std types instead of boost for Gaussian sampling #2351

Merged
merged 5 commits into from
Oct 19, 2023

Conversation

Shobuj-Paul
Copy link
Contributor

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

codecov bot commented Sep 10, 2023

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
Contributor Author

Shobuj-Paul commented Sep 11, 2023

@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
Contributor Author

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
Copy link
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
Copy link
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 😅

*Need to compare function implementations
*Find an equivalent implementation for variate_generator
@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
12 checks passed
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modernize random sampling in STOMP
3 participants