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

Add median and gaussian 1D filters as an optional step for gradient based edge detection #424

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@vrozin
Copy link
Contributor

commented Apr 13, 2019

Hi @ihhub !

After some time that I spent with this issue, I have the following:

  1. As per your request, issue #414 gets fixed in findEdgePoints() function. Positive and negative edge vectors, if have any data, get cleared.

  2. Since not having much experience with Gaussian distribution, I did some readings (source) and I noticed that when calculating weighs for kernel, denominator of the exp() function should be 2*(sigma^2), but the algorithm in filtering.cpp works with double sigma; was the squire omitted intentional or it's a typo?

  3. I updated the find() function just so it receives optional type of filtering (None, Median, or Gaussian), kernel size, and sigma (applicable for Gaussian filtering only).

  4. To be honest, I was a bit confused about how to place the 1D values in the filter vector in modified GetGaussianKernel() function.
    The 2D version allocates as much space as we have for the image and distributes the weights at the centre of this vector (square of size kernelSize * 2 + 1). So, basically, we have a nice zero-padded matrix, which we can convolute against our image to filter it. But I wasn't sure how I'd place 1D kernel on it. If I were to combine vertical and horizontal vectors like so (but normalized)

0 0 0 0 0
0 0 1 0 0
0 1 2 1 0
0 0 1 0 0
0 0 0 0 0

then there is basically no run-time advantage when comparing to 2D version. So, I narrowed down the size of the filter to kernelSize and it should look like so [1 2 1] (or [0.25 0.5 0.25] - normalized version). So, we basically don't need to pass image's width and height in the function?
But then, since the kernel is not padded, I needed to write a convolution function. I chose to implement circular convolution (circularConvolution()) since it takes care of pixels at borders of an image as well.

It seems like an image gets blurred, and I tried to use various kernel sizes to test it with. I'm not quite sure it is what you wanted though, but I can modify it anytime :)

  1. If you don't mind, I tried to implement another way of calculating the kernel with usage of Pascal's triangle (source).
    I ran both and this one gives a bit faster results while also providing 1D Gaussian image filtering. The code snippet to test them both is right below.
#include <ctime>
#include <iostream>
...
applyFiltering<_Type>(...) {
...
case (GAUSSIAN):
            Image_Function::Image dst1, dst2;
            std::vector<_Type> kernel;

            std::clock_t timeSpan1 = std::clock();
            getPascalTriangleLine<_Type>(kernel, filterKernelSize - 1u);
            circularConvolution<_Type>(image, dst1, kernel);
            std::cout << "Time 1: " << (std::clock() - timeSpan1) / (double)CLOCKS_PER_SEC << '\n';

            std::vector<_Type> kernel2;

            std::clock_t timeSpan2 = std::clock();
            getGaussianKernel1D<_Type>(kernel2, image.width(), image.width(), filterKernelSize, sigma);
            circularConvolution<_Type>(image, dst2, kernel2);
            std::cout << "Time 2: " << (std::clock() - timeSpan2) / (double)CLOCKS_PER_SEC << '\n';

            return image;
        }

Fixes #346
Fixes #414

@ihhub ihhub added this to the Release milestone Apr 13, 2019

@ihhub

This comment has been minimized.

Copy link
Owner

commented Apr 13, 2019

Hi @vrozin , thanks for your big efforts! I left few comments regarding this pull request.

Give me please some time and I show you the code for Gaussian filter, it's pretty simple so we don't need so much changes :)

@ihhub

This comment has been minimized.

Copy link
Owner

commented Apr 13, 2019

First, we need to have a function which creates 1D Gaussian filter:

    template< typename _Type >
    std::vector<_Type> getGaussianKernel1D( uint32_t kernelSize, _Type sigma )
    {
        if ( kernelSize == 0 || sigma < 0 )
            throw imageException("Incorrect input parameters for 1D Gaussian filter kernel");

        std::vector<_Type> filter( kernelSize * 2 + 1, 0.0f );

        const _Type pi = 3.1415926536f;
        const _Type doubleSigma = sigma * 2;
        const _Type doubleSigmaPiInv = 1.0f / (doubleSigma * pi);

        _Type * x = filter.data();
        _Type sum = 0;

        const int32_t start = -static_cast<int32_t>(kernelSize);
        const int32_t end = static_cast<int32_t>(kernelSize) + 1;

        for ( int32_t pos = start; pos < end; ++pos, ++x ) {
            *x = doubleSigmaPiInv * exp(-static_cast<_Type>(pos * pos) / doubleSigma);
            sum += *x;
        }

        const _Type normalization = 1.0f / sum;
        x = filter.data();

        for ( int32_t pos = start; pos < end; ++pos, ++x )
            (*x) *= normalization;

        return filter;
    }

Then inside

template<typename _Type>
    void findEdgePoints( std::vector < _Type > & positive, std::vector < _Type > & negative, std::vector < int > & data,
                         std::vector < int > & first, std::vector < int > & second, const EdgeParameter & edgeParameter, bool forwardDirection )

function we need to check the type of filter and apply it on data as first step inside this function.
The rest of things I will write later.

@ihhub ihhub changed the title Issue 346 Add median and gaussian 1D filters as an optional step for gradient based edge detection Apr 15, 2019

@ihhub
Copy link
Owner

left a comment

Hi @vrozin would you mind to have a look into my comments? We could do step by step changes :)

Show resolved Hide resolved examples/windows/edge_detection/example_edge_detection.vcxproj Outdated
@@ -202,6 +203,12 @@ namespace
void findEdgePoints( std::vector < _Type > & positive, std::vector < _Type > & negative, std::vector < int > & data,
std::vector < int > & first, std::vector < int > & second, const EdgeParameter & edgeParameter, bool forwardDirection )
{
if (!positive.empty() && !negative.empty())

This comment has been minimized.

Copy link
@ihhub

ihhub Apr 15, 2019

Owner

We don't need to clear vectors here.

@@ -354,13 +515,29 @@ void EdgeParameter::verify() const
}

void EdgeDetectionHelper::find( EdgeDetection<double> & edgeDetection, const PenguinV_Image::Image & image, uint32_t x, uint32_t y, uint32_t width, uint32_t height,

This comment has been minimized.

Copy link
@ihhub

ihhub Apr 15, 2019

Owner

Please clear positive and negative edge vectors in this function.

}

void EdgeDetectionHelper::find( EdgeDetection<float> & edgeDetection, const PenguinV_Image::Image & image, uint32_t x, uint32_t y, uint32_t width, uint32_t height,
const EdgeParameter & edgeParameter )
const EdgeParameter & edgeParameter, Filter filterToApply, uint32_t filterKernelSize, float sigma )

This comment has been minimized.

Copy link
@ihhub

ihhub Apr 15, 2019

Owner

Please clear positive and negative edge vectors in this function.

@@ -50,31 +51,39 @@ struct EdgeParameter
void verify() const; // self-verification that all parameters are correct
};

// Optional parameter to apply before looking for first and second gradients on an image (findEdgePoints)
enum Filter

This comment has been minimized.

Copy link
@ihhub

ihhub Apr 15, 2019

Owner

Please put filterType enumeration inside EdgeParameter structure and revert changes for input parameters for EdgeDetection classes. We need to setup filter parameters within EdgeParameter structure.

This comment has been minimized.

Copy link
@vrozin

vrozin Apr 16, 2019

Author Contributor

Actually, makes sense. I don't know why I didn't think about it in the first place :)
Will do.

This comment has been minimized.

Copy link
@ihhub

ihhub Apr 17, 2019

Owner

Sure. Let's do small changes which I commented about. Step by step :)

@@ -3,6 +3,7 @@
#include <vector>
#include "image_buffer.h"
#include "math/math_base.h"
#include "filtering.h"

This comment has been minimized.

Copy link
@ihhub

ihhub Apr 15, 2019

Owner

We don't need this file inclusion here.

{
EdgeDetectionHelper::find( *this, image, x, y, width, height, edgeParameter );
EdgeDetectionHelper::find( *this, image, x, y, width, height, edgeParameter, filterToApply, filterKernelSize, sigma );

This comment has been minimized.

Copy link
@ihhub

ihhub Apr 17, 2019

Owner

What if we clear our 2 vector here just before calling EdgeDetectionHelper? I think it would make code cleaner as well.

@ihhub

This comment has been minimized.

Copy link
Owner

commented Apr 22, 2019

Hey @vrozin why don't we commit a fix for #414 which is just 2 lines of code and then merge with all changes in master branch? I think fixing small items step by step would be easier ;)

@vrozin

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

Hi @ihhub ! My apologies about the delay.
I am currently working on college project, which is due on Apr 30th. If the issues are not urgent, would you you mind if I continue with them on 30th/1st?

@ihhub

This comment has been minimized.

Copy link
Owner

commented Apr 23, 2019

@vrozin no worries, education is very important :) Take your time and come back to this issue once you have time.
I will just fix #414 and #435 issues as they are somewhat bugs. I'll put you as a reviewer to see the changes ;)

@vrozin

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

Hi @ihhub! The proposed changes have been applied. Waiting for the appveyor to finish the check...
Due to erasing #include "filtering.h from src/edge_detection.h file, there's no source of the Median filter to use on an image though.

@ihhub

This comment has been minimized.

Copy link
Owner

commented May 8, 2019

Hi @vrozin I'll take a look in a while.

@ihhub

This comment has been minimized.

Copy link
Owner

commented May 21, 2019

Hi @vrozin my apologies as I was busy and I totally forgot about your PR. It seems like you use different line ending technique which causes web-editor think that it's full replacement of existing code. This really complicates code review. Which text editor do you use?

@vrozin

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

Oh no worries at all.
Normally, I use VS 2015/2017. But as I was checking the edge_detection.cpp in Vim, I saw that only in this particular file I had gotten ^M carriage returns everywhere. So, in hopes to change the line endings to the ones similar in all other files, I got rid of carriage returns.
Do you want me to revert everything there as it was?

@ihhub

This comment has been minimized.

Copy link
Owner

commented May 25, 2019

@vrozin lets fix ^M issue in a separate pull request to properly track changes. I'll create different issue for this. Please revert such change for now.

@ihhub

This comment has been minimized.

Copy link
Owner

commented May 25, 2019

I'll take a good look at the pull request today once I get free.

@ihhub

This comment has been minimized.

Copy link
Owner

commented May 25, 2019

Hi @vrozin , maybe we should make one step back and add only median filter as of now as it's implementation should be straightforward. I pointed few places where we need to change the code. What if we make just a separate pull request for median filter only based on latest code from master branch? Currently this pull request have conflicts too which are needed to be solved.

What do you think? :)

@ihhub

This comment has been minimized.

Copy link
Owner

commented May 30, 2019

Hi @vrozin are you going to work on this pull request? :) Negative answer is also acceptable if you have no time

@vrozin

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

Hi @ihhub! My apologies about the delay, I totally forgot to reply to your previous message.
Yes, absolutely. I will look into it over the weekend.
So, if to sum up:

  1. Revert the change in line endings for now (^M).
  2. Make a PR to add Median filter to the project.

In regards of #2, where would you like me to add the code to? Would it be somewhere in filtering.cpp next to other algorithms?

@ihhub

This comment has been minimized.

Copy link
Owner

commented Jun 2, 2019

Hi @vrozin No-no. What I meant is to do changes for edge detection but only with median filter for it. Changes should be only in edge detection. 1D median filter (just use partial sort). I think it should be a reasonable step. We don't want an extra dependency from filtering.cpp for edge detection code.

@ihhub

This comment has been minimized.

Copy link
Owner

commented Jun 2, 2019

@vrozin how median filter should work: we have a separate function which accepts a vector (original pixel intensity values: std::vector < int > & data input value in findEdgePoints function) and median filter size. Such new function is called first in findEdgePoints body. The function accept the vector by reference, i.e. modifies it. Inside the function we create a temporary array of filter size which we use for partial sort. So the idea is very similar to implementation of Median() filter in filtering.cpp but input data is 1D. I think it should be just few lines of code :)

@ihhub

This comment has been minimized.

Copy link
Owner

commented Jun 2, 2019

My apology for late reply and such long pull request review. I hope we find a solution very soon :)

@vrozin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

I do like the idea of breaking down the task into smaller parts, let's see if it'll make it work out faster :)
The PR for median filter has been submitted.

@vrozin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

I used circular approach when calculating median values. Don't like an idea of leaving values at the borders as is, but can change it if this is preferred way (judging on 2D implementation).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.