search

JuliaImages / ImageBinarization.jl

A Julia package of algorithms for analyzing images and automatically binarizing them into background and foreground.
MIT License
35 stars 8 forks source link

refactor and enhance AdaptiveThreshold method #30

Closed johnnychen94 closed 5 years ago

johnnychen94 commented 5 years ago

This PR co-operates #29 with some further noteworthy enhancement:

TODO:

RFC:

According to my understanding, window_size requires the information of the to-be-binarized image, which makes it not the intrinsic property of AdaptiveThreshold.

So the proper usage IMO is:

f = AdaptiveThreshold(percentage = 15)
binarize(img, f, window_size)

We could add some one-liners to make the usage more convenient, i.e.,

(f::AdaptiveThreshold)(out, img) = f(out, img, recommended_size(img))

by doing this the default window_size is automatically chosen according to the input image instead of hardcoded 32.

Check psnr as an example, where peak_value doesn't belong to PSNR.

zygmuntszpak commented 5 years ago

@rjww You may be interested in these proposed changes too.

zygmuntszpak commented 5 years ago

According to my understanding, window_size requires the information of the to-be-binarized image, which makes it not the intrinsic property of AdaptiveThreshold.

The AdaptiveThreshold algorithm adapts the binarization threshold based on the intensity distribution of a sliding region-of-interest in the image. The size of the region-of-interest is determined by the window_size. I reckon that in the context of thresholding, a window_size is something that is intrinsic to an adaptive method since the existence of a window is what often differentiates global thresholding from adaptive local thresholding. A sensible choice of window_size does indeed depend on the size of the image. I would have put it as a property of AdaptiveThreshold, but I would like to understand your point of view better since I may have misunderstood your reasoning.

rename: recommend_size --> recommended_size

We went with recommend_size since we thought of it as an active process. You specify an image and it recommends the appropriate size. recommended_size is what I would have given to the variable name, i.e. recommended_size = recommend_size(img).

johnnychen94 commented 5 years ago

a window_size is something that is intrinsic to an adaptive method since the existence of a window is what often differentiates global thresholding from adaptive local thresholding.

After a second thought, speaking of the concept, I think you're right that window_size does differ the behavior of different AdaptiveThrehold objects.

I'm thinking about not re-creating a new AdatptiveThrehold object when we want to binarize a sequence of images, i.e., to avoid the following usage:

f1 = AdaptiveThreshold(window_size = recommend_size(img1))
img1_01 = binarize(img1, f)

f2 = AdaptiveThreshold(window_size = recommend_size(img2), percentage=15)
img2_02 = binarize(img2, f)

...

This won't be a performance issue for any non-trivial algorithms. But the usage isn't so concise to me.

Also, I think manually calling recommend_size is tedious and trivial. The following line makes users no longer need to call it by themselves.

(f::AdaptiveThreshold)(out, img) = f(out, img, recommended_size(img))

and the usage becomes much easier:

f = AdaptiveThreshold()
img1_01 = binarize(img1, f)

img2_02 = binarize(img2, f)

f_30 = AdaptiveThreshold(percentage=30)
img3_03 = binarize(img3, f_30)

I guess this's a trade-off between conceptually-right and engineering-friendly, how do you think?

johnnychen94 commented 5 years ago

We went with recommend_size since we thought of it as an active process.

Good point. I was mainly writing python codes in last two weeks and there're functions like repeated, sorted that create a new object instead of doing an in-place operation repeat and sort. But it seems that Julia doesn't use this naming convention.

Another change I forgot to mention: to avoid unnecessary memory allocation, I choose to not creating an Array{Gray{Bool}, 2} and return it. For example, all the following usage should be permitted and doesn't create a new array IMO:

binarize!(out::AbstractArray{Bool}, img::AbstractArray{<:Number}, f)::AbstractArray{Bool}
binarize!(out::AbstractArray{Bool}, img::AbstractArray{<:Colorant}, f)::AbstractArray{Bool}
binarize!(out::AbstractArray{Gray{Bool}}, img::AbstractArray{<:Number}, f)::AbstractArray{Gray{Bool}}
binarize!(out::AbstractArray{Gray{Bool}}, img::AbstractArray{<:Colorant}, f)::AbstractArray{Gray{Bool}}

I'm planning to do it in the coming commits.

johnnychen94 commented 5 years ago

Update:

This PR is ready to be reviewed.

Actually, n-D array isn't supported yet since boxdiff is limited to 2D array.

@zygmuntszpak Regardless of the window_size issue, can you have a look at the style of docstring and test case? If you're okay with it I can merge it and refactor other algorithms in the same way. Then we can create another PR for window_size if you like it.

About docstring, I treat it as a cheat sheet explaining how to use it and what we can expect on its output, instead of the full explanation on its theoretical and coding details. Unfortunately, I find many of your docstrings are too long to read and understand even though they're written in good quality.

codecov[bot] commented 5 years ago

Codecov Report

Merging #30 into api will increase coverage by 20.86%. The diff coverage is 93.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##              api      #30       +/-   ##
===========================================
+ Coverage   16.56%   37.42%   +20.86%     
===========================================
  Files          19       20        +1     
  Lines         163      171        +8     
===========================================
+ Hits           27       64       +37     
+ Misses        136      107       -29
Impacted Files Coverage Δ
src/ImageBinarization.jl 100% <ø> (ø) :arrow_up:
src/adaptive_threshold.jl 100% <100%> (+100%) :arrow_up:
src/deprecations.jl 66.66% <100%> (+66.66%) :arrow_up:
src/compat.jl 33.33% <33.33%> (ø)
src/integral_image.jl 75% <0%> (+6.25%) :arrow_up:
src/BinarizationAPI/binarize.jl 100% <0%> (+100%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ca471b8...8e1a2f4. Read the comment docs.

zygmuntszpak commented 5 years ago

Also, I think manually calling recommend_size is tedious and trivial. The following line makes users no longer need to call it by themselves.

The recommend_size function was only meant to be a convenience and a rule-of-thumb. There may be many instances where you don't want to use the recommended size, so we should still permit the user to specify a manual size. If I understood correctly your proposed solution will always use the recommend_size function?

Perhaps one possible path is to let window_size be a function

Base.@kwdef struct AdaptiveThreshold{T <: Function} <: AbstractImageBinarizationAlgorithm
           percentage::Float64 = 15.0
           window_size::T = recommend_size
end

and then have a constructor which takes a number and creates an anonymous function that returns the user-specified size.

AdaptiveThreshold(percentage::Number, window_size::Number) = AdaptiveThreshold(percentage, x-> window_size)
johnnychen94 commented 5 years ago

If I understood correctly your proposed solution will always use the recommend_size function?

No. The signature of binarize for AdaptiveThreshold is:

    binarize(img, f [, window_size])

and usage examples are:

img = testimage("lena")
f = AdaptiveThreshold()
binarize(img, f) # infer window_size according to img

binarize(img, f, 16) # explicitly provide window_size
zygmuntszpak commented 5 years ago

About docstring, I treat it as a cheat sheet explaining how to use it and what we can expect on its output, instead of the full explanation on its theoretical and coding details. Unfortunately, I find many of your docstrings are too long to read and understand even though they're written in good quality.

I have a different philosophy. I don't intend for the docstrings to be a cheat sheet, but rather an explanation of what the algorithm does, what assumptions it makes, what effect different options have etc. I'm not suggesting that the documentation that we have written attains all of those goals, but I would rather move towards that goal than away from it. I believe that the theoretical details are also useful if one wants to dig into the actual code and follow the implementation. I have kept the structure of the documentation headings in this package the same as in my other package: https://zygmuntszpak.github.io/ImageContrastAdjustment.jl/dev/ because I want to create a consistent look and feel.

The detailed explanation is meant to go in the section "Details" so that anyone that is not interested can just skip it. I understand that this causes a lot of scrolling if you consult the documentation from the REPL. I read the documentation predominantly in a browser, in Jupyter or in Atom, so I am prioritizing that user experience.

The headings "Options" are supposed to give you a "cheatsheet" of what you can fiddle with etc. I suppose that often the options can be written in more "bullet" point style. Certainly the ones in this package can be written in bullet points. There will be cases, however, where an explanation of an algorithm option may not fit as a single sentence in a bullet point. I had therefore opted to write everything as a sentence in order not to have to mix styles. We could use bullet points in this package instead, but I would still like to keep the top-level headings: Output, Details, Options etc in accordance with the other package. The inspiration for detailed documentation comes from Mathematica documentation, e.g.: https://reference.wolfram.com/language/ref/ImagePyramid.html

johnnychen94 commented 5 years ago

Update:

Deprecations:

Any further comment?

zygmuntszpak commented 5 years ago

Its looking great, thank you very much Johnny!

johnnychen94 commented 5 years ago

As the very first PR in #29, I'm merging this now. The future PRs will be created parallelly using this as a template.