[Morphology][Tasks] introduce generic fillholes algorithm#119
[Morphology][Tasks] introduce generic fillholes algorithm#119ThomasRetornaz wants to merge 2 commits intoJuliaImages:masterfrom
Conversation
Todos: * speedup binary implem
mkitti
left a comment
mkitti
left a comment
There was a problem hiding this comment.
Thank you for the contribution. Could you look over my comments and suggestions?
benchmark/benchmarks.jl
Outdated
| # Usage: | ||
| # julia benchmark/run_benchmarks.jl | ||
| Usage: | ||
| julia benchmark/run_benchmarks.jl |
src/ImageMorphology.jl
Outdated
| include("clearborder.jl") | ||
| include("extreme_filter.jl") | ||
| include("extremum.jl") | ||
| include("filholes.jl") |
| 0 1 0 0 0 1 0 | ||
| 0 1 0 0 0 1 0 | ||
| 0 1 1 1 1 1 0 | ||
| 0 0 0 0 0 0 0 |
There was a problem hiding this comment.
Could you add a test where the 0s at the border are not a single connected component? Also the case where there are no zeros on the border m. Also add a test for multiple interior holes.
There was a problem hiding this comment.
Note that we could say nothing for "holes" in the border, anyway we don't have acess to underlyting image domain, are these really holes
So as other image processing framework we don't fill hole touching the borders
| @@ -0,0 +1,58 @@ | |||
| """ | |||
| fillhole(img; [dims]) | |||
There was a problem hiding this comment.
Change the name of the file to match the base of the function, please
| return _fillhole!(out, img, se) | ||
| end | ||
|
|
||
| function _fillhole!(out, img, se) |
There was a problem hiding this comment.
We should consider typing the arguments here and extracting the number of dimensions and element type from the AbstractArray{T,N}.
There was a problem hiding this comment.
I agree i could but why?
The function is generic, the speedup of binary cases could be to specialize mreconstruct for binary cases
Do you want to specialize this function at fillhole level ?
test/fillholes.jl
Outdated
|
|
||
| # in place | ||
| out = similar(img) | ||
| out = fillhole!(out, img) |
There was a problem hiding this comment.
Do we need the left side assignment here?
src/ImageMorphology.jl
Outdated
| feature_transform, | ||
| distance_transform, | ||
|
|
||
| #clearborder |
src/ImageMorphology.jl
Outdated
| feature_transform, | ||
| distance_transform, | ||
|
|
||
| #clearborder |
src/ImageMorphology.jl
Outdated
| #clearborder | ||
| clearborder, | ||
|
|
||
| #fillhole |
| end | ||
|
|
||
| function fillhole!(out, img, se) | ||
| return _fillhole!(out, img, se) |
There was a problem hiding this comment.
Why do we need the underscore prefix? Could we combine the function below and this one to a single signature?
There was a problem hiding this comment.
The prefix correspon to the most internal call, you will see this pattern in the whole project, something like "private" implem
But yes you're right in this special case its not relevant
* fix naming * add more tests
johnnychen94
left a comment
johnnychen94
left a comment
There was a problem hiding this comment.
I don't know the algorithm details, but how the codes are written is exactly what I would do by myself. The approval is for this.
Extensive test cases are appreciated -- I always believe that tests are the only final guards for long-term correctness.
| outerrange = CartesianIndices(map(i -> 1:i, dimensions)) | ||
| innerrange = CartesianIndices(map(i -> (1 + 1):(i - 1), dimensions)) | ||
| for i in EdgeIterator(outerrange, innerrange) | ||
| tmp[i] = 0 |
There was a problem hiding this comment.
For generic programming: tmp[i] = zero(T) where I assume T = eltype(img)
The difference is very small but noteworthy:
tmp[i] = 0would be valid if there's a conversion from0(Int) toeltype(tmp)tmp[i] = zero(T)would be valid as long aszero(T)is defined (which is true for almost all number-like types)
Why we should prefer the zero(T) version is that: not all types (should) support (implicit) conversion from Int. The following is an JuliaImages example, but you can quickly come up with many like it:
julia> using ImageCore
julia> zero(HSV)
HSV{Float32}(0.0f0,0.0f0,0.0f0)
julia> HSV(0)
ERROR: in ccolor, no automatic conversion from Int64 and HSV
Stacktrace:
[1] ccolor(#unused#::Type{HSV}, #unused#::Type{Int64})
@ ColorTypes ~/.julia/packages/ColorTypes/1dGw6/src/traits.jl:410
[2] convert(#unused#::Type{HSV}, c::Int64)
@ ColorTypes ~/.julia/packages/ColorTypes/1dGw6/src/conversions.jl:74
[3] HSV(x::Int64)
@ ColorTypes ~/.julia/packages/ColorTypes/1dGw6/src/types.jl:464
[4] top-level scope
@ REPL[3]:14 participants
Todos: