Skip to content

Fix blob detector insertion sort (issue #12244)#14954

Merged
opencv-pushbot merged 1 commit intoopencv:3.4from
rafagjordana:fix_blob_detection_insertion_sort
Jul 2, 2019
Merged

Fix blob detector insertion sort (issue #12244)#14954
opencv-pushbot merged 1 commit intoopencv:3.4from
rafagjordana:fix_blob_detection_insertion_sort

Conversation

@rafagjordana
Copy link
Copy Markdown

@rafagjordana rafagjordana commented Jul 1, 2019

resolves #12244

This pullrequest changes

This PR addresses a small bug in the implementation of insertion sort inside the blob detector, as found in issue #12244.

Reproducible example

Here is a dumbed down version of the bug and its solution that you can run on e.g. cpp.sh (using doubles instead of circles but the idea is the same). The fix is simple really, the implementation of insertion sort is comparing the wrong variable in the while loop.

#include <iostream>
#include <string>
#include <vector>
#include <algorithm>


std::vector<double> broken_insert_sort(std::vector<double> centers, double new_num) {
    centers.push_back(new_num);
    
    size_t k = centers.size() - 1;
    while( k > 0 && centers[k] < centers[k-1]) // Wrong comparison here
    {
        centers[k] = centers[k-1];
        k--;
    }
    centers[k] = new_num;
    return centers;
}

std::vector<double> proper_insert_sort(std::vector<double> centers, double new_num) {
    centers.push_back(new_num);
    
    size_t k = centers.size() - 1;
    while( k > 0 && new_num < centers[k-1]) // Proper comparison here
    {
        centers[k] = centers[k-1];
        k--;
    }
    centers[k] = new_num;
    return centers;
}

void print_vec(const std::vector<double>& vec) {
    std::cout << "[";
    std::for_each(vec.begin(), vec.end(), [](double v) {
        std::cout << v << ", ";
    });
    std::cout << "]" << std::endl;
}

int main()
{
    std::vector<double> centers{3.0, 4.0, 5.0, 6.0};
    
    auto bugged = broken_insert_sort(centers, 2.0); // This is the version of insertion sort found in blobdetector.cpp
    auto fixed = proper_insert_sort(centers, 2.0); // This includes the fix (regular insertion sort)
    
    print_vec(bugged);
    print_vec(fixed);
}

@alalek
Copy link
Copy Markdown
Member

alalek commented Jul 2, 2019

Thank you for the contribution!

As a "bugfix" this patch should go into 3.4 branch first. We will merge changes from 3.4 into master regularly (weekly/bi-weekly).

So, please:

  • change "base" branch of this PR: master => 3.4 (use "Edit" button near PR title)
  • rebase your commits from master onto 3.4 branch. For example:
    git rebase -i --onto upstream/3.4 upstream/master
    (check list of your commits, save and quit (Esc + "wq" + Enter)
    where upstream is configured by following this GitHub guide and fetched (git fetch upstream).
  • push rebased commits into source branch of your fork (with --force option)

Note: no needs to re-open PR, apply changes "inplace".

Copy link
Copy Markdown
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

Nice catch! Thank you 👍

@opencv-pushbot opencv-pushbot merged commit 9a24886 into opencv:3.4 Jul 2, 2019
opencv-pushbot pushed a commit that referenced this pull request Jul 2, 2019
@alalek alalek mentioned this pull request Jul 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants