Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #242 +/- ##
==========================================
+ Coverage 91.86% 92.17% +0.30%
==========================================
Files 12 12
Lines 1647 1648 +1
==========================================
+ Hits 1513 1519 +6
+ Misses 134 129 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
johnnychen94
left a comment
johnnychen94
left a comment
There was a problem hiding this comment.
Docs and tests are needed.
Some docstrings in ImageFiltering are written in Julia pre-1.0 age, so you can follow the guideline in https://docs.julialang.org/en/v1/manual/documentation/#man-documentation. I personally prefer a verbose docstring, e.g., the docstring for Gabor in #235. You can also follow how unit tests are written in that PR.
Style fix Co-authored-by: Johnny Chen <johnnychen94@hotmail.com>
| #Citation | ||
| Selesnick, Ivan W., and C. Sidney Burrus. "Generalized digital Butterworth filter design." IEEE Transactions on signal processing 46.6 (1998): 1688-1694. | ||
| """ | ||
|
|
There was a problem hiding this comment.
Extra empty line here would cause ?Kernel.butterworth giving nothing.
| end | ||
|
|
||
| """ | ||
| butterworth(n,Wn,ls{tuple}) -> k |
There was a problem hiding this comment.
indentation, and :: for type annotation.
| butterworth(n,Wn,ls{tuple}) -> k | |
| butterworth(n, Wn, ls::Dims) -> k |
|
|
||
| """ | ||
| butterworth(n,Wn,ls{tuple}) -> k | ||
| Returns a multidimensional dimensional Butterworth kernel contained in an OffsetArray(::Matrix{Float64}) |
There was a problem hiding this comment.
one extra new line after the signature.
| 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 |
There was a problem hiding this comment.
You don't need indentation for the markdown list
| - `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 |
|
|
||
| - `n` is the order of the filter | ||
| - `Wn` is the normalized cutoff frequency | ||
| - `ls` is the size of the kernel |
There was a problem hiding this comment.
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?
| @. 1/(1+(sqrt(2)-1)*(df(R)/Wn)^nn) | ||
| end | ||
|
|
||
| butterworth(n::Real, Wn::Real, ls::Integer) = butterworth(n, Wn, (ls,ls)) |
There was a problem hiding this comment.
This convenient method only makes sense when butterworth is 2D only; otherwise, we should not add it.
| @test Kernel.butterworth(a,b,(3,3)) == Kernel.butterworth(a,b,3) | ||
| @test Kernel.butterworth(a,b,(4,4)) == Kernel.butterworth(a,b,4) |
There was a problem hiding this comment.
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.
| Selesnick, Ivan W., and C. Sidney Burrus. "Generalized digital Butterworth filter design." IEEE Transactions on signal processing 46.6 (1998): 1688-1694. | ||
| """ | ||
|
|
||
| function butterworth(n::Real, Wn::Real, ls::Tuple{Integer,Integer}) |
There was a problem hiding this comment.
It looks like if we do
| 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}).
2 participants
In response to feature request #240
Please let me know if there are any fixes to be done on the code.