Revisiting the Images API (exposure.jl)

[zygmuntszpak]

As a continuation of #767 I thought I'd start the process of drafting some tentative changes to the Images API to facilitate further discussion.

One aspect that we need to pin down is how we are going to design the API to support the execution of algorithms that take advantage of the GPU. The ImageFiltering package which makes use of the ComputationalResources.jl package to dispatch to an implementation most suitable for a particular device.

exposure.jl

I'll use the exposure.jl file as an example of how we might want to restructure our API.
That file used to export the following functions:

Exposure : `imhist`, `histeq`, `adjust_gamma`, `histmatch`, `imadjustintensity`, `imstretch`, `imcomplement`, `clahe`, `cliphist`

My intention is to introduce the following types:

abstract type AbstractHistogramOperation end
struct Equalization <: AbstractHistogramOperation end
struct Matching <: AbstractHistogramOperation end
struct Stretching <: AbstractHistogramOperation end
struct Clipping <: AbstractHistogramOperation end
struct CLAHE <: AbstractHistogramOperation end

and to have a functions

 adjust_histogram(op::Equalization, params...)
 adjust_histogram(op::Matching, params...)
 adjust_histogram(op::Stretching, params...)
 adjust_histogram(op::Clipping, params...)
 adjust_histogram(op::CLAHE, params...)

I thought of adding struct GammaCorrection <: AbstractHistogramOperation and then
adjust_histogram(op::GammaCorrections, params...) but we could also have adjust_gamma(params...) and I'm not sure what people would prefer here.

Additionally, we would have the build_histogram(params...) functions. I presume that the
imadjustintensity and imstretch could be absorbed as sub-types of the aforementioned AbstractHistogramOperation types.

Taking into consideration that one could envisage GPU based implementations of these algorithms, perhaps function arguments ought to follow a: what, were, how pattern. That is, the first parameter designates which operation to dispatch on, the second parameter designates whether a CPU/GPU based implementation is desired and the remaining parameters are the standard input parameters to the function. The second parameter could by default be set to CPU (and hence optional). The remaining parameters could be keyword parameters, with the exception that we don't make the "obvious" inputs keyword parameters, i.e. the input image/images.

Since there are several .jl files in the repository, I thought I'd open separate issues for each of the .jl files in Images. Hopefully that will help focus the discussion by breaking the tasks down.

Since I have already started rewriting some of the functions in exposure.jl and extending the documentation I decided I may as well finish all of the functions in that file. The next major one that I intend to tackle soon is CLAHE. I'm aiming for a more concise implementation by leveraging the interpolations.jl package.

[timholy]

This is a very well-conceived proposal! I don't have any serious reservations. In ImageFiltering there isn't really the what option, but it does allow you to pass the eltype of the output as an argument:

julia> methods(imfilter)
# 11 methods for generic function "imfilter":
[1] imfilter(img::AbstractArray, kernel, args...) in ImageFiltering at /home/tim/.julia/dev/ImageFiltering/src/imfilter.jl:5
[2] imfilter(::Type{T}, img::AbstractArray{TI,N} where N, kernel::AbstractArray{TK,N} where N, args...) where {T<:Integer, TI<:Integer, TK<:Integer} in ImageFiltering at /home/tim/.julia/dev/ImageFiltering/src/imfilter.jl:13
[3] imfilter(::Type{T}, img::AbstractArray, kernel::Union{Laplacian, Union{IIRFilter{T}, AbstractArray{T,N} where N, ReshapedOneD{T,N,Npre,V} where V<:(AbstractArray{T,1} where T) where Npre where N, ReshapedOneD{T,N,Npre,V} where V<:IIRFilter where Npre where N} where T}, args...) where T in ImageFiltering at /home/tim/.julia/dev/ImageFiltering/src/imfilter.jl:10
[4] imfilter(::Type{T}, img::AbstractArray, kernel::Tuple, border::AbstractString, args...) where T in ImageFiltering at /home/tim/.julia/dev/ImageFiltering/src/imfilter.jl:22
[5] imfilter(::Type{T}, img::AbstractArray, kernel::Tuple, border::ImageFiltering.AbstractBorder, args...) where T in ImageFiltering at /home/tim/.julia/dev/ImageFiltering/src/imfilter.jl:27
[6] imfilter(::Type{T}, img::AbstractArray, kernel::Tuple, args...) where T in ImageFiltering at /home/tim/.julia/dev/ImageFiltering/src/imfilter.jl:18
[7] imfilter(r::ComputationalResources.AbstractResource, img::AbstractArray, kernel, args...) in ImageFiltering at /home/tim/.julia/dev/ImageFiltering/src/imfilter.jl:32
[8] imfilter(r::ComputationalResources.AbstractResource, ::Type{T}, img::AbstractArray, kernel::Union{IIRFilter{T}, AbstractArray{T,N} where N, ReshapedOneD{T,N,Npre,V} where V<:(AbstractArray{T,1} where T) where Npre where N, ReshapedOneD{T,N,Npre,V} where V<:IIRFilter where Npre where N} where T, args...) where T in ImageFiltering at /home/tim/.julia/dev/ImageFiltering/src/imfilter.jl:36
[9] imfilter(r::ComputationalResources.AbstractResource, ::Type{T}, img::AbstractArray, kernel::Tuple) where T in ImageFiltering at /home/tim/.julia/dev/ImageFiltering/src/imfilter.jl:42
[10] imfilter(r::ComputationalResources.AbstractResource, ::Type{T}, img::AbstractArray, kernel::Tuple, border::AbstractString) where T in ImageFiltering at /home/tim/.julia/dev/ImageFiltering/src/imfilter.jl:46
[11] imfilter(r::ComputationalResources.AbstractResource, ::Type{T}, img::AbstractArray, kernel::Tuple, border::ImageFiltering.AbstractBorder) where T in ImageFiltering at /home/tim/.julia/dev/ImageFiltering/src/imfilter.jl:50

That may be the closest analog of what, and it comes after the AbstractResource (the where). I am not opposed to changing this, just pointing it out.

[juliohm]

Thank you @zygmuntszpak for the thoughtful and exciting proposal. I have a quick question. Are the params in the line

adjust_histogram(op::Equalization, params...)

the parameters of the Equalization operation? Couldn't the parameters be encapsulated in the struct?

struct Equalization <: AbstractHistogramOperation
  param1
  param2
end

[zygmuntszpak]

Hi @juliohm, yes you are right they could in principle go as parameters into anEqualization type. It comes down to a choice between

adjust_histogram(Equalization(10, 15), img)
adjust_histogram(Equalization(), img, 10, 15)

In some instance the function may take several parameters, i.e CLAHE

adjust_histogram(CLAHE(nbins = 256, xblocks = 8, yblocks = 8, clip = 3),  img)
adjust_histogram(CLAHE(), img, nbins = 256, xblocks = 8, yblocks = 8, clip = 3)

I guess at first I thought that the second variant was more "readable" but I'm not so sure anymore.
One thing that I would amend from what I proposed is not to have the where as a second parameter, but rather to have it a keyword parameter resource, and which by default is set to CPU().

[johnnychen94]

I came to a similar conclusion in Towards consistent style, part 1: a naming guide where I emphasized avoiding function name suffixed by implementation detail, e.g., adjust_histogram_local.

I prefer adjust_histogram(Equalization(10, 15), img) than adjust_histogram(Equalization(), img, 10, 15), this makes the function signiture more clear and self-explained.

At present I can think of three reasons:

1. It's very easy for future extension to follow into the same API, e.g., a batch-version implementation adjust_histogram(::Equalization, ::ImageDataset) instead of adjust_histogram(::Equalization, ::ImageDataset, params...). -- We don't need to write docs for each of params of each implementation/wrappers
2. users can easily find out that nbins controls the behavior of CLAHE just from the function signature without reading detailed docs, so that they can immediately decide if he needs to dig into the implementation details of CLAHE
3. Personally, if there're 9 params in adjust_histogram, I prefer to take care of them 3 by 3 by 3 instead of 9 in a bunch. The second style losses this ability to divide function call into small pieces.

[johnnychen94]

ImageBinarization, ImageQualityIndexes, ImageEdgeDetection are now written with this design. I don't think there's anything more that needs to be discussed in this issue so I'm closing it.