[features] change SHOT estimation to a dynamic schedule #6400
Conversation
benchmarks/CMakeLists.txt
Outdated
|
|
||
| PCL_ADD_BENCHMARK(features_shot FILES features/shot.cpp | ||
| LINK_WITH pcl_io pcl_search pcl_features | ||
| ARGUMENTS "${PCL_SOURCE_DIR}/test/milk.pcd") |
There was a problem hiding this comment.
Lets used milk_cartoon_all_small_clorox instead? Its a full kinect scan with 300.000 points with color?
There was a problem hiding this comment.
milk.pcd is only a supset of the one I mention, with 13700 points. Still more than bun0.pcd(about 400 pts), but not that much still really.
|
@larshg I tried |
There was a problem hiding this comment.
Pull request overview
This PR changes SHOT (Signature of Histograms of OrienTations) feature estimation to use OpenMP dynamic scheduling instead of the default static scheduling, addressing issue #5785 which identifies that loops with varying iteration times (like neighbor searches) can benefit from dynamic scheduling. The PR also adds benchmarks to measure the performance impact of different chunk sizes.
Changes:
- Added configurable chunk_size parameter to SHOTEstimationOMP and SHOTColorEstimationOMP constructors for dynamic scheduling
- Modified OpenMP pragmas to use
schedule(dynamic, chunk_size_)instead of default static scheduling - Added comprehensive benchmarks for both SHOT352 and SHOT1344 descriptors with varying chunk sizes
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| features/include/pcl/features/shot_omp.h | Added chunk_size parameter to constructors and member variables for both SHOTEstimationOMP and SHOTColorEstimationOMP classes |
| features/include/pcl/features/impl/shot_omp.hpp | Updated OpenMP parallel for directives to use dynamic scheduling with configurable chunk size |
| benchmarks/features/shot.cpp | New benchmark file testing SHOT352 and SHOT1344 estimation with different chunk sizes (64, 128, 256) |
| benchmarks/CMakeLists.txt | Added features_shot benchmark to the build system |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * \param nr_threads the number of hardware threads to use (0 sets the value back to automatic) | ||
| * \param chunk_size PCL will use dynamic scheduling with this chunk size. Setting it too low will lead to more parallelization overhead. Setting it too high will lead to a worse balancing between the threads. | ||
| */ | ||
| explicit SHOTEstimationOMP (unsigned int nr_threads = 0, int chunk_size = 256) |
There was a problem hiding this comment.
The explicit keyword is inconsistent with similar OMP feature estimation classes in the codebase. Other classes like NormalEstimationOMP (normal_3d_omp.h:77) and PrincipalCurvaturesEstimation (principal_curvatures.h:82) do not use explicit for their constructors with default parameters. For consistency with the established pattern, consider removing explicit from this constructor.
There was a problem hiding this comment.
@kai-waang Is there a reason why you made this constructor explicit?
| std::cerr << "No test files given. Please provide a PCD file paths for SHOT352 " | ||
| "and SHOT1344 benchmarks." |
There was a problem hiding this comment.
The error message says "Please provide a PCD file paths" (plural "paths") but the code only checks for and uses a single file argument (argv[1]). Either update the error message to say "file path" (singular) or clarify that one file is sufficient for both benchmarks.
| std::cerr << "No test files given. Please provide a PCD file paths for SHOT352 " | |
| "and SHOT1344 benchmarks." | |
| std::cerr << "No test files given. Please provide a PCD file path to use for " | |
| "both SHOT352 and SHOT1344 benchmarks." |
3 participants
As discussed in #5785, change SHOT estimation to a dynamic schedule. Benchmarks for SHOT estimation is also added.