-
Notifications
You must be signed in to change notification settings - Fork 8
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
Comments
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? |
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; Not sure if this is a good practice. Because if this is, then we can argue things like |
another possible solution is to take different export strategies, i.e.,
|
At the moment we export only two methods: 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 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()) |
In case you misunderstand me, I don't mean to vote for APIs like 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:
Adding docstring is absolutely sufficient for users to understand what But I'm thinking about the need to narrow down the scope of exported symbols to make the APIs of My thought on the role of If you're thinking of 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. |
@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. |
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. |
This is an naive implementation of my proposal, but I have to admit that using 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) |
perhaps we could support the API such as binarize(img, :Otsu) which calls the default constructor |
closed by #29 |
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 forImages.jl
users.Perhaps we need an intermediate wrapper method
binarize_method
to generateBinarizationAlgorithm
The text was updated successfully, but these errors were encountered: