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

feat: add compositor strel_chain and strel_product #92

Merged
merged 2 commits into from
Jun 13, 2022
Merged

Conversation

johnnychen94
Copy link
Member

@johnnychen94 johnnychen94 commented Jun 3, 2022

This provides two helper functions strel_chain and strel_product
that one can use to compose multiple SEs into a bigger one.

The optimization on extreme_filter, however, is not a part of this PR.

This resolves part of #75

This provides two helper functions `strel_chain` and `strel_product`
that one can use to compose multiple SEs into a bigger one.

The optimization on `extreme_filter`, however, is not a part of this PR.
Comment on lines +86 to +92
se1 = centered(rand(Bool, 3, 3))
se2 = centered(rand(Bool, 3, 3))
img = rand(64, 64)
out_pipe = f(f(img, se1), se2)
out_se = f(img, strel_chain(se1, se2))
R = CartesianIndices(img)[2:(end - 1), 2:(end - 1)] # inner region
@test out_pipe[R] == out_se[R]
Copy link
Member Author

Choose a reason for hiding this comment

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

@ThomasRetornaz do you know what's the common way to handle this boundary condition if we want to provide optimized implementation?

I can think of two ways:

  • padding the image and crop it afterwards
  • use the fast implementation to compute the inner region, and use the slow implementation to computer the boundary region

Copy link
Collaborator

Choose a reason for hiding this comment

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

You could thind both in the litterature
its common to use (always?) padded images (eg name image + border)
My point is that in c/C++ always add padding leads to some headache for other algorithms, but its not the case in julia
But for instance my implem of erode/dilate diamond to need an explicit padding
so a ON Demand padding should be great.
For other algorithm like reconstruction i will try both and choose the fastest one (note that in that case the radius of the Se equals 1)
Padding could lead to faster simd access but for all algorithm we could not define a proper fill(border,value)
For erode/dilateits ok (eg TypeEltMax,TypeEltMin) for a median filter its better to split the algorithm
Regards
TR

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the comment; always insightful.

a ON Demand padding should be great.

In Julia, we have lazy PaddedArrays.jl, but it's not performant because every getindex introduces a bounds check even for the inner region.
This is why ImageFiltering.jl provides a padarray implementation to eagerly pad arrays.

In this sense, the on-demand padding seems more like the second method I mentioned:

  • for the inner region, we don't need to consider boundary issues and thus there's no need to do a bound check. This permits fast SIMD implementation.
  • for the boundary region, use the lazily padded array. In this case, the boundary check is unavoidable. But since the boundary is relatively small, it is okay to have a slow version.

I don't want to rush out a quick implementation for morphology -- we already have ImageFiltering.padarray and PaddedViews.jl and TiledIterations.jl
I think I'll spend more time playing with a new package for this and solve some old issues(e.g., JuliaArrays/PaddedViews.jl#5 and separating ImageFiltering.padarray to a thin package)

@johnnychen94 johnnychen94 merged commit 9530ec1 into master Jun 13, 2022
@johnnychen94 johnnychen94 deleted the jc/chain branch June 13, 2022 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants