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

Merge FPFHEstimationOMP into FPFHEstimation #4281

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

shrijitsingh99
Copy link
Contributor

Merges both classes and removes code duplication

TODO:

  1. Update docs
  2. Update docstrings if needed
  3. Discuss and update deprecation policy accordingly

@shrijitsingh99 shrijitsingh99 added changelog: deprecation Meta-information for changelog generation changelog: enhancement Meta-information for changelog generation module: features priority: gsoc Reason for prioritization labels Jul 18, 2020
FPFHEstimationOMP (unsigned int nr_threads = 0) : nr_bins_f1_ (11), nr_bins_f2_ (11), nr_bins_f3_ (11)
{
feature_name_ = "FPFHEstimationOMP";
PCL_DEPRECATED_HEADER(1, 13, "Use <pcl/features/fpfh.h> instead.")
Copy link
Member

Choose a reason for hiding this comment

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

The number here is for removal. Release in 1.12 and removal in 1.13 doesn't sound great. Make it the 1.14 at least

output[idx].histogram[d] = std::numeric_limits<float>::quiet_NaN ();

output.is_dense = false;
if (input_->is_dense || isFinite ((*input_)[(*indices_)[idx]])) {
Copy link
Member

Choose a reason for hiding this comment

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

Please benchmark this. Should not have adverse effects, but just to be sure


output.is_dense = false;
if (input_->is_dense || isFinite ((*input_)[(*indices_)[idx]])) {
if (this->searchForNeighbors(
Copy link
Member

Choose a reason for hiding this comment

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

Outdent the block please

@kunaltyagi kunaltyagi added the needs: more work Specify why not closed/merged yet label Aug 7, 2020
@stale
Copy link

stale bot commented Sep 6, 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 Sep 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: deprecation Meta-information for changelog generation changelog: enhancement Meta-information for changelog generation module: features needs: more work Specify why not closed/merged yet priority: gsoc Reason for prioritization status: stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants