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

Filter specification: an alternative to binarize #26

Closed
johnnychen94 opened this issue Apr 4, 2019 · 8 comments
Closed

Filter specification: an alternative to binarize #26

johnnychen94 opened this issue Apr 4, 2019 · 8 comments

Comments

@johnnychen94
Copy link
Member

johnnychen94 commented Apr 4, 2019

abstract type BinarizationAlgorithm end
struct Otsu <: BinarizationAlgorithm end

The current design is

  • (Design A)
binarize(algo::Otsu, img) = println("binarize img with Otsu")

The usage is

julia> algo = Otsu()
julia> binarize(algo, img)
binarize img with Otsu

Actually, there's an alternative design, which is commonly used in Flux.jl AFAIK

  • (Design B)
(binarizer::Otsu)(img) = println("binarize img with Otsu")

The usage is:

julia> binarizer = Otsu()
julia> binarizer(img)
binarize img with Otsu

In design A, two different algorithms are connected in two ways: BinarizationAlgorithm and binarize. In design B, they're only connected by BinarizationAlgorithm.

According to the naming guide https://github.com/JuliaImages/Images.jl/issues/767, we should use binarize

for functions that compute something or perform an operation, start with a verb (e.g., warp)

But I guess there would be some cases Design B is more natural.

Any ideas?

@zygmuntszpak
Copy link
Member

zygmuntszpak commented Apr 4, 2019

I like that a lot! (Design B)

@zygmuntszpak
Copy link
Member

I will work on refactoring the code incorporating all the suggestions from this and related issues.

@johnnychen94
Copy link
Member Author

johnnychen94 commented Apr 4, 2019

The idea behind design B is that we treat Otsu() as a filter instead of an algorithm, and since a filter filters some signal, it doesn't necessarily need to be a verb

More generally, we can support both of the following two specifications -- I can't see a reason to support one and deprecate another one.

  • Filter specification (Design B)
abstract type Filter end
abstract type Binarizer <: Filter end
struct Otsu <: Binarizer end

function (filter::Otsu)(img)
...
end
  • functional specification (Design A)
abstract type Algorithm end
abstract type BinarizationAlgorithm end
struct Otsu <: BinarizationAlgorithm end

function binarize(algo::Otsu, img)
...
end

@johnnychen94
Copy link
Member Author

johnnychen94 commented Apr 4, 2019

Based on current codebase, it would be quite trivial to support the other

(filter::BinarizationAlgorithm)(img) = binarize(filter, img)

But a more natural way to write code, IMO, is design B,

# ImageBinarization.jl
abstract type Filter end
abstract type Binarizer <: Filter end
binarize(filter::Binarizer, img) = filter(img)

# otsu.jl
struct Otsu <: Binarizer end
function (filter::Otsu)(img)
...
end

Mostly, it's just about how we start to think and build things up from the beginning.

Except, there does exist a minor difference: binarize doesn't need to spread over all the implementation files in design B.

@zygmuntszpak
Copy link
Member

I guess we should consider how many other image processing operations can be cast in this form. Ideally, we would like to have a consistent way of doing things for different operations, i.e. edge detection, contrast adjustment etc.

@johnnychen94
Copy link
Member Author

ping @timholy @Evizero @juliohm

@johnnychen94 johnnychen94 changed the title Alternative to binarize Filter specification: an alternative to binarize Apr 5, 2019
@johnnychen94
Copy link
Member Author

johnnychen94 commented Apr 9, 2019

Except, there does exist a minor difference: binarize doesn't need to spread over all the implementation files in design B.

Just came up with another difference:

  • If some_name(img) works, the only thing we can know about it is that it's callable in the sense isempty(methods(some_name)) === false(not necessary Function or Base.Callable)`
  • if binarize(some_name, img) works, we immediately knows that some_name isa BinarizationAlgorithm

So to be more accurate,

binarize(some_name, img)

is equivalent to

if some_name isa BinarizationAlgorithm
    some_name(img)
end

For this reason, the filter specification might be slightly more bug-prone and slightly less-understandable to end-user. And it makes documenting easier, we don't need the workaround mentioned in JuliaImages/Images.jl#790 (comment)

If we agree on this, then a better way of design might be:

  1. implement algorithm details using filter specification -- leave implementation details to detailed type. (Functional specification is also good to do this, but just not so clean compared to the filter specification.)
  2. export api using functional specification as a recommended usage, i.e., remain binarize as usual.

johnnychen94 added a commit that referenced this issue Jul 31, 2019
Changes:

* refactor the codebase using the functor API discussed in #26
* enhance the API by introducing a submodule `BinarizationAPI`
* add in-place function `binarize!`
* support Color3 inputs
* add more test codes
* slightly enhance the documentation

Breaking changes (Deprecated in 0.3):

* swap the argument order discussed in #23 ( d1f8309)
* unexport `recommend_size` in favor of #41, i.e., `AdaptiveThreshold(img)` instead of `recommend_size(img)` ( PR: #30 #45)
* made `window_size` of `AdaptiveThreshold` not an optional argument. ( PR: #45 )
@johnnychen94
Copy link
Member Author

closed by #29

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

No branches or pull requests

2 participants