Added 2D Gabor filter

[arijitkar98]

I have added the function gabor in kernel.jl that implements the 2D gabor kernel. I am not clear about how to add tests for checking the function.

[arijitkar98]

Ping @timholy @Evizero

[zygmuntszpak]

Thanks for your contribution. To add some tests you can proceed as follows.

1. In the ImageFilter/test directory create a new file called gabor.jl.
2. Write your tests in the gabor.jl file. You can use gradient.jl as a reference.
3. Modify runtests.jl by adding a line include("gabor.jl")
4. Test the ImageFiltering package by running Pkg.test("ImageFiltering")

Do you have a reference you could add for the paper which you based your implementation on?

[arijitkar98]

Hi @zygmuntszpak I implemented the filter taking help from wikipedia and opencv, so should I write the references given in wikipedia as reference for the filter?

[zygmuntszpak]

@arijitkar98 I've had a quick glance at the literature to see if I can find a paper in which your particular parametrization appears verbatim. N. Petkov uses it frequently in his work as early as the 1990s. I couldn't find that particular parametrization in the original work of Daugman (the references on wikipedia). Hence, I suggest you use citation [1] instead.

Having glanced at some of the articles, it seems that you have only implemented the real part of the complex Gabor filter. Shouldn't the function also return the corresponding complex part of the filter?
I noticed that N. Petkov seems to work exclusively with the real part. In skimming over some of the publications it might be because it was the real part of the Gabor filter that turned out to model the visual cortex of monkeys. On the other hand, the answer to the question When should we use the full Gabor Function and when do we use only the real part? suggests that the imaginary part of the Gabor function is also very useful.

[1] N. Petkov and P. Kruizinga, “Computational models of visual neurons specialised in the detection of periodic and aperiodic oriented visual stimuli: bar and grating cells,” Biological Cybernetics, vol. 76, no. 2, pp. 83–96, Feb. 1997. doi.org/10.1007/s004220050323

[arijitkar98]

@zygmuntszpak Thank you for the input 😃

Should I make two separate functions for the real and complex parts or just one which takes an extra argument?

[zygmuntszpak]

Since the Gabor filter is traditionally defined as a complex sinusoid modulated by a Gaussian envelope, I think it makes sense for the function to return both the real and imaginary component. That is, I wouldn't necessarily write the API so that one has gabor_real() and gabor_imaginary() functions. Rather, I suggest that you modify your current function so that it returns a tuple of kernels (kernel_real, kernel_imaginary).

If the user is interested in just the real part then they can just assign the imaginary part to a dummy variable. For example, kernel_real, _ = gabor(k_size, σ, θ, λ, γ, ψ).

You may also want to write another helper function (not exposed to the user) such as validate_gabor which verifies that the parameters that have been passed are suitable. For example, currently the code is open to a division by zero if γ is zero. Similarly, σ must always be a positive non-zero number.

Thank you for all your efforts!

[codecov]

Codecov Report

Merging #57 into master will increase coverage by 0.21%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #57      +/-   ##
==========================================
+ Coverage   90.84%   91.05%   +0.21%     
==========================================
  Files           8        8              
  Lines        1070     1096      +26     
==========================================
+ Hits          972      998      +26     
  Misses         98       98

Impacted Files	Coverage Δ
src/ImageFiltering.jl	100% <ø> (ø)	⬆️
src/kernel.jl	100% <100%> (ø)	⬆️
src/border.jl	96.87% <0%> (ø)	⬆️
src/imfilter.jl	89.31% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0186e50...890cca1. Read the comment docs.

[arijitkar98]

@zygmuntszpak I have modified the gabor filter function as requested and have added required tests for the same.

[zygmuntszpak]

@arijitkar98 Looks great, and thank you for compiling such thoughtful tests.

I have some further small suggestions. Instead of writing error("Invalid Input Parameters!") perhaps you can be more specific and say something along the lines of error("The parameters σ, λ and γ must be positive numbers.")

I noticed that you choose to make validate_gabor an inner function. My initial though was to move the parameter input checking out of the gabor function because it just felt like a neater solution. If you are going to put parameter validation code inside the gabor function then perhaps you don't even need an inner function. Presumably you could just write

if !(σ>0 && λ>0 && γ>0) 
   error("The parameters σ, λ and γ must be positive numbers.")`
end

which would be more concise.

Should we test that size_x and size_y are positive as well?

Finally, to ensure maximum code coverage you could add a few further tests for which σ, λ and γ are zero so that the code associated with the error condition gets executed.

[Evizero]

Thank you both for putting so much effort and thought into this. I am sorry that I have found such little time to look into recent PRs and issues. That said it looks like @zygmuntszpak provided much more thoughtful feedback than I could have thought of.

One small suggestion I would add is to instead of error(...) use throw(ArgumentError(...)) in this instance.

[arijitkar98]

@zygmuntszpak @Evizero I have modified the validate_gabor function, keeping it outside of the main gabor function.

For size_x and size_y, I have added a warning whenever the sizes are not positive, taking such an input value (6*σx+1) in function that will keep almost whole of the gaussian in the kernel.

I have also added the test cases for the errors.

Thank you for the code reviews 😃

[arijitkar98]

@Evizero Are there any more changes required before this PR can be merged?

[Evizero]

can you make those into bullet points, and surround the parameters in backticks?

e.g.

- `size_x`, `size_y` denote the size of the kernel

etc

[Evizero]

one small docstring related comment. Other than that it looks good to me and i trust the judgement of @zygmuntszpak here

[arijitkar98]

Ping @Evizero

[Evizero]

Thanks!