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

export limited number of symbols? #24

Closed
johnnychen94 opened this issue Mar 20, 2019 · 10 comments
Closed

export limited number of symbols? #24

johnnychen94 opened this issue Mar 20, 2019 · 10 comments

Comments

@johnnychen94
Copy link
Member

I'm wondering if it is necessary to export all implemented methods: https://github.com/zygmuntszpak/ImageBinarization.jl/blob/master/src/ImageBinarization.jl#L61-L72

If this package is reexported by Images.jl, which I think eventually will be, then all these names: Ostu,Balanced,Yen become be meaningless for Images.jl users.

Perhaps we need an intermediate wrapper method binarize_method to generate BinarizationAlgorithm

@zygmuntszpak
Copy link
Member

Hi Johnny,

I'm not sure I understand what you mean. Please could you elaborate how the wrapper method would work? How would the end-user select a particular binarization algorithm?

@johnnychen94
Copy link
Member Author

johnnychen94 commented Mar 20, 2019

I don't know how to write this in julia now since it's for a long time not using it. But the idea is to

binarize(binarize_method("Otsu"), img)

The way to call it is terrible but I think you can get what I mean; binarize_method constructs a BinarizationAlgorithm instance. -- Can we do it in a julian way without export these Otsu methods?

Not sure if this is a good practice. Because if this is, then we can argue things like binarize("Otsu", img) are good as well.

@johnnychen94 johnnychen94 changed the title export limited number of functions? export limited number of methods? Mar 20, 2019
@johnnychen94
Copy link
Member Author

another possible solution is to take different export strategies, i.e.,

  • export all these symbols if using ImageBinarization -- users know that they want to binarize images
  • export limited number of symbols if using Images, i.e., binarize only -- if we do this, perhaps it's better to support binarize(::AbstractArray)

@zygmuntszpak
Copy link
Member

At the moment we export only two methods: binarize and recommend_size. The remaining exports are actually types:

abstract type BinarizationAlgorithm end
struct Otsu <: BinarizationAlgorithm end
struct Polysegment <: BinarizationAlgorithm end
struct UnimodalRosin <: BinarizationAlgorithm end
struct MinimumIntermodes <: BinarizationAlgorithm end
struct Moments <: BinarizationAlgorithm end
struct Intermodes <: BinarizationAlgorithm end
struct MinimumError <: BinarizationAlgorithm end
struct Balanced <: BinarizationAlgorithm end
struct Yen <: BinarizationAlgorithm end
struct Entropy <: BinarizationAlgorithm en

I don't quite understand what the problem is with having many exported types. If your concern is that people might do an API search and spot the Otsu type before they come across the binarize(algorithm::Otsu, img::AbstractArray) method, then perhaps we can just add docstrings for the Otsu type so that if the user types ?Otsu at the REPL they are directed to the relevant binarize function?

Could you please elaborate on why you prefer (1) and (2) versus (3)?

# (1)
binarize(binarize_method("Otsu"), img)
#(2)
binarize("Otsu", img)
#(3) 
binarize(Otsu(), img)

Actually, in light of #23 we should be discussing

# (1)
binarize(img, binarize_method("Otsu"))
#(2)
binarize(img, "Otsu")
#(3) 
binarize(img, Otsu())

@johnnychen94 johnnychen94 changed the title export limited number of methods? export limited number of symbols? Mar 21, 2019
@johnnychen94
Copy link
Member Author

johnnychen94 commented Mar 21, 2019

In case you misunderstand me, I don't mean to vote for APIs like binarize("Otsu", img); indeed, I think we should avoid kwargs-like arguments as possible as we can -- this just isn't the julia way. I just want to know if there's another julia way of constructing an instance of Otsu without export Otsu.


This is what I'm actually concerned about: those types become meaningless in a larger namescope.

One aspect of my concern is as you said:

people might do an API search and spot the Otsu type before they come across the binarize(algorithm::Otsu, img::AbstractArray) method.

Adding docstring is absolutely sufficient for users to understand what Otsu is, only when they want to know it.

But I'm thinking about the need to narrow down the scope of exported symbols to make the APIs of Images.jl more concise and clearer. If there are 500 hundred symbols in Images.jl, nobody will ?Otsu it unless they know they (might) want to use it, which is a dilemma. -- binarize(::BinarizationAlgorithm, ::AbstractArray) is a good way for developers to maintain the package, but IMO not a good way for users to call it in a broader namescope, i.e., Images.jl.


My thought on the role of Images.jl is an entrance to different related image processing packages. (@timholy @Evizero @juliohm How do you think of this as the principle of Images.jl?) (Update: I found a related issue here JuliaImages/ImageFeatures.jl#46)

If you're thinking of Images.jl in the same way, and if all the potential packages export their algorithm types, then another aspect of my concern is potential symbol conflicts. For example, there definitely will be several TVL1 types, e.g., one from ImageDenoise.jl, one from ImageDeblur.jl, one from ImageSuperresolution.jl, and one from ImageWhatever.jl


I just think we need to be aware of this problem, but I have no idea of what good solution for this would be. We can definitely export these symbols for ease of use, but I think ease of understanding is of more importance.

@juliohm
Copy link
Member

juliohm commented Mar 21, 2019

@johnnychen94 I understand your concerns, but I think exporting symbols is not that problematic in practice. We hope that we will be able to always pick a unique symbol for a method. If there is something common across different Images.jl sub-modules, that probably means that this concept should be moved somewhere else in the project like a ImageBase.jl package. Symbol duplication is an issue that JuliaImages maintainers can handle easily I guess. We just need to agree on a good unique name for each symbol exported by one of Images.jl submodules.

@johnnychen94
Copy link
Member Author

johnnychen94 commented Mar 22, 2019

Thanks for the reply.

I came up with this idea/concern when I was writing my proposal, and I thought it worth pointing out. At some point, a comprehensive collection of documentation and tutorials will definitely solve this "problem".

As a former philosophy student, I'm quite interested in these abstract and "meaningless" topics 😂and frequently I'm behaving stubbornly, but I do enjoy changing ideas and thoughts with all of you -- if you're not feeling disturbed. If there is, I'm sorry for any possible offense I've made unconsciously.

I'll keep this issue open in case I have more thoughts on it.

@johnnychen94
Copy link
Member Author

johnnychen94 commented Mar 30, 2019

This is an naive implementation of my proposal, but I have to admit that using String argument is a terrible way.

function binarization_algorithm(alg_name::String, args...)::ImageBinarization.BinarizationAlgorithm
  alg_name = Symbol(alg_name)
  non_alg_error = ArgumentError("Binarization algorithm $alg_name is not implemented yet.")
  try
    (@eval ImageBinarization.$(alg_name) <: ImageBinarization.BinarizationAlgorithm) || throw(non_alg_error)
  catch err
    if isa(err, UndefVarError)
      throw(non_alg_error)
    else
      rethrow(err)
    end
  end
  return @eval $(alg_name)($args...)
end
julia> binarization_algorithm("Int")
ERROR: ArgumentError: Binarization algorithm Int is not implemented yet.
Stacktrace:
 [1] binarization_algorithm(::String) at ./REPL[20]:5
 [2] top-level scope at none:0

julia> binarization_algorithm("Non")
ERROR: ArgumentError: Binarization algorithm Non is not implemented yet.
Stacktrace:
 [1] binarization_algorithm(::String) at ./REPL[20]:8
 [2] top-level scope at none:0

julia> binarization_algorithm("Otsu")
Otsu()

julia> binarization_algorithm("AdaptiveThreshold", 17, 32)
AdaptiveThreshold(17, 32)

@johnnychen94
Copy link
Member Author

perhaps we could support the API such as

binarize(img, :Otsu)

which calls the default constructor Otsu()?

@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

3 participants