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

[filters] Improve performance of median filter by using nth_element #4360

Merged
merged 1 commit into from
Aug 30, 2020

Conversation

mvieth
Copy link
Member

@mvieth mvieth commented Aug 27, 2020

nth_element is faster than partial_sort, and should be used for median finding

@Morwenn
Copy link
Contributor

Morwenn commented Aug 27, 2020

Suggestion:

auto middle_it = vals.begin () + vals.size () / 2;
std::nth_element (vals.begin (), middle_it, vals.end ());
float new_depth = *middle_it;

@kunaltyagi kunaltyagi changed the title Use nth_element in median filter [filters] Improve performance of median filter by using nth_element Aug 27, 2020
@kunaltyagi kunaltyagi added the changelog: enhancement Meta-information for changelog generation label Aug 27, 2020
@mvieth
Copy link
Member Author

mvieth commented Aug 28, 2020

Suggestion:

auto middle_it = vals.begin () + vals.size () / 2;
std::nth_element (vals.begin (), middle_it, vals.end ());
float new_depth = *middle_it;

@kunaltyagi @SergioRAgostinho What's your opinion on this? Should I do this?

@SergioRAgostinho
Copy link
Member

It's up to you. I'm okay with both.

nth_element is faster than partial_sort, and should be used for median finding
@mvieth
Copy link
Member Author

mvieth commented Aug 30, 2020

I added the suggestion. The failed test seems unrelated.

@SergioRAgostinho SergioRAgostinho merged commit d4312fc into PointCloudLibrary:master Aug 30, 2020
@mvieth mvieth deleted the median_filter branch August 30, 2020 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: enhancement Meta-information for changelog generation module: filters
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants