Skip to content

Add Butterworth Filter #242

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.

Sign up for GitHub

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

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

Add Butterworth Filter #242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f00d04e
add butterworth codes
ashwinn-v Jan 17, 2022
b6a98ca
add butterworth func
ashwinn-v Jan 17, 2022
228207e
Update src/kernel.jl
ashwinn-v Jan 17, 2022
e10c1b0
minor changes
ashwinn-v Jan 17, 2022
a848ed0
Docs and Tests
ashwinn-v Jan 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added .DS_Store
View file
Open in desktop
Binary file not shown.
2 changes: 1 addition & 1 deletion src/ImageFiltering.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ ArrayLike{T} = Union{ArrayType{T}, AnyIIR{T}}

include("kernel.jl")
using .Kernel
using .Kernel: Laplacian, reflect, ando3, ando4, ando5, scharr, bickley, prewitt, sobel, gabor, moffat
using .Kernel: Laplacian, reflect, ando3, ando4, ando5, scharr, bickley, prewitt, sobel, gabor, moffat, butterworth

NDimKernel{N,K} = Union{AbstractArray{K,N},ReshapedOneD{K,N},Laplacian{N}}

Expand Down
20 changes: 20 additions & 0 deletions src/kernel.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,26 @@ moffat(α::Real, β::Real) = moffat(α, β, ceil(Int, (α*2*sqrt
sum(x.^2)
end

"""
butterworth(n,Wn,ls{tuple}) -> k
Copy link
Member

Choose a reason for hiding this comment

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

indentation, and :: for type annotation.

Suggested change
butterworth(n,Wn,ls{tuple}) -> k
butterworth(n, Wn, ls::Dims) -> k

Returns a multidimensional dimensional Butterworth kernel contained in an OffsetArray(::Matrix{Float64})
Copy link
Member

Choose a reason for hiding this comment

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

one extra new line after the signature.

Suggested change
Returns a multidimensional dimensional Butterworth kernel contained in an OffsetArray(::Matrix{Float64})
Returns a multidimensional dimensional Butterworth kernel contained in an OffsetArray(::Matrix{Float64})


- `n` is the order of the filter
- `Wn` is the normalized cutoff frequency
- `ls` is the size of the kernel
Comment on lines +516 to +518
Copy link
Member

Choose a reason for hiding this comment

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

You don't need indentation for the markdown list

Suggested change
- `n` is the order of the filter
- `Wn` is the normalized cutoff frequency
- `ls` is the size of the kernel
- `n` is the order of the filter
- `Wn` is the normalized cutoff frequency
- `ls` is the size of the kernel

Copy link
Member

Choose a reason for hiding this comment

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

why you use ls for kernel size instead of sz? The former seems to come from the math equation of butterworth kernel. Can you also add the equation to the docstring so that people immediately knows what n, Wn, ls means and how they affects the results?

#Citation
Selesnick, Ivan W., and C. Sidney Burrus. "Generalized digital Butterworth filter design." IEEE Transactions on signal processing 46.6 (1998): 1688-1694.
"""

Copy link
Member

Choose a reason for hiding this comment

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

Extra empty line here would cause ?Kernel.butterworth giving nothing.

Suggested change

function butterworth(n::Real, Wn::Real, ls::Tuple{Integer,Integer})
Copy link
Member

Choose a reason for hiding this comment

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

It looks like if we do

Suggested change
function butterworth(n::Real, Wn::Real, ls::Tuple{Integer,Integer})
function butterworth(n::Real, Wn::Real, ls::NTuple{N,Integer}) where N

we immediately get a N-dimensional implementation as the docstring says: "multidimensional dimensional Butterworth kernel". Can you check this and if it works, add 1d and 3d tests? Also don't forget to update the docstring OffsetArray(::Matrix{Float64}).

ws = map(n->(ceil(Int,n)>>1), ls)
R = CartesianIndices(map(w->IdentityUnitRange(-w:w), ws))
nn = 2*n
@. 1/(1+(sqrt(2)-1)*(df(R)/Wn)^nn)
end

butterworth(n::Real, Wn::Real, ls::Integer) = butterworth(n, Wn, (ls,ls))
Copy link
Member

Choose a reason for hiding this comment

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

This convenient method only makes sense when butterworth is 2D only; otherwise, we should not add it.


"""
reflect(kernel) --> reflectedkernel

Expand Down
8 changes: 8 additions & 0 deletions test/specialty.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,14 @@ using ImageFiltering: IdentityUnitRange
fwhm = ceil(Int, (α*2*sqrt(2^(1/β) - 1)) * 4)
@test Kernel.moffat(α, β) == Kernel.moffat(α, β, (fwhm, fwhm))
end

@testset "butterworth" begin
a = rand()
b = rand()
@test Kernel.butterworth(a,b,(3,3)) == Kernel.butterworth(a,b,3)
@test Kernel.butterworth(a,b,(4,4)) == Kernel.butterworth(a,b,4)
Comment on lines +245 to +246
Copy link
Member

Choose a reason for hiding this comment

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

We also need some numerical tests here, can you test against one or two results generated from other frameworks that has butterworth implemented or manual calculated results? Also do remember to add a comment on how the references are generated.

We also need to test that the kernel axis are "centered". For instance, the axes should be (-1:1, -1:1) when kernel size is (3, 3). Also need to test the even case as well.

end

end

nothing