Merge NormalEstimationOMP into NormalEstimation #4279
Conversation
shrijitsingh99
force-pushed
the
normal_3d
branch
from
71d3d47 to
f99736c
Compare
shrijitsingh99
force-pushed
the
normal_3d
branch
from
f99736c to
51539cf
Compare
shrijitsingh99
added
changelog: deprecation
| shared(output) \ | ||
| firstprivate(nn_indices, nn_dists) \ | ||
| num_threads(threads_) \ | ||
| if (threads_ != -1) |
There was a problem hiding this comment.
If setNumberOfThreads() isn't called and no value is supplied to the constructor, the algorithm won't be run? Shouldn't it just default to one thread?
Can we maybe add { } for better readability?
There was a problem hiding this comment.
Moving the if to be closer to the top might be better
| @@ -258,11 +227,12 @@ namespace pcl | |||
| using PointCloudConstPtr = typename Feature<PointInT, PointOutT>::PointCloudConstPtr; | |||
|
|
|||
| /** \brief Empty constructor. */ | |||
| NormalEstimation () | |||
| NormalEstimation (unsigned int nr_threads = -1) | |||
There was a problem hiding this comment.
A separate ctor instead of an overloaded default ctor is better
There was a problem hiding this comment.
Default arguments have some counter-intuitive behavior:
- Future change in default value doesn't affect user's pre-compiled code. So code working with a.b.0 and a.b.1 can have different behavior if the defaults are changed (which is not a ABI/API breakage)
and some nice properties in refactoring:
- Allows the overloaded ctor to protected, giving access only to derived class (if we don't deprecate the OMP class)
| shared(output) \ | ||
| firstprivate(nn_indices, nn_dists) \ | ||
| num_threads(threads_) \ | ||
| if (threads_ != -1) |
There was a problem hiding this comment.
Moving the if to be closer to the top might be better
| public: | ||
| PCL_MAKE_ALIGNED_OPERATOR_NEW | ||
| }; | ||
|
|
||
| template <typename PointInT, typename PointOutT> | ||
| using NormalEstimationOMP PCL_DEPRECATED(1, 12, "use NormalEstimation instead") = NormalEstimation<PointInT, PointOutT>; |
There was a problem hiding this comment.
@SergioRAgostinho Would you prefer this approach or the approach where the setNumberOfThreads is visible only in the derived class made for OMP?
There was a problem hiding this comment.
If the point is to try to enforce consistency, the latter. Only OMP classes need to worry about how many threads are running.
Edit: My idea was not to deprecate the OMP version. The OMP class should be the one exposing controls to tweak the number of threads.
I would defer getting rid of classes with OMP suffix after the introduction of executors for each class.
There was a problem hiding this comment.
@shrijitsingh99 Could you please undeprecate the OMP class?
There was a problem hiding this comment.
I didn't understand the discussion. What do you mean by derived class for OMP?
To clarify, these PRs are meant as basework for integration with executors. The ideas was to get the design of this combined class approved then integrate with executors then merge with master.
Apologize for that, I should have clarified it early on.
There was a problem hiding this comment.
then integrate with executors then merge with master.
If I understood Sergio correctly, OMP class should not be deprecated till the executors are integrated.
What do you mean by derived class for OMP?
The OMP class derives from the base class. It adds only a ctor and overloads where needed to expose controls to OMP which are hidden in the base class.
Benefits:
- doesn't break API to users till executors are ready
- removes 2 implementations and any chance of them diverging
- still has 1 class for you to add executors to
There was a problem hiding this comment.
Oh okay so that's just removing the deprecation warning then, right?
There was a problem hiding this comment.
- Remove deprecation warnings
- Let OMP class handle the num_thread related stuff (setting/changing, etc.)
|
Marking this as stale due to 30 days of inactivity. Commenting or adding a new commit to the pull request will revert this. |
Merges both classes and removes code dupilcation
TODO: