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

dispatch binarize on HistogramThresholding.ThresholdAlgorithm #67

Closed
johnnychen94 opened this issue Jun 22, 2020 · 2 comments · Fixed by #70
Closed

dispatch binarize on HistogramThresholding.ThresholdAlgorithm #67

johnnychen94 opened this issue Jun 22, 2020 · 2 comments · Fixed by #70

Comments

@johnnychen94
Copy link
Member

It's a bit awkward that two closely related packages have name conflicts:

julia> using HistogramThresholding

julia> using ImageBinarization
[ Info: Precompiling ImageBinarization [cbc4b850-ae4b-5111-9e64-df94c024a13d]

julia> Yen
WARNING: both ImageBinarization and HistogramThresholding export "Yen"; uses of it in module Main must be qualified
ERROR: UndefVarError: Yen not defined

A simple and intuitive solution to this is to reuse the type HistogramThresholding.Yen and directly dispatch binarize on it. But this requires some code organization changes and it may have other unexpected side effects.

@zygmuntszpak
Copy link
Member

Yes, I guess it didn't occur to me at the time that one would want to load both packages in the same problem domain.

I'm not sure how to resolve the conflict of the AbstractImageBinarizationAlgorithm type which makes sense in ImageBinarization, and the AbstractHistogramThresholding type (currently just called ThresholdAlgorithm) which makes sense for HistogramThresholding.

Is the name conflict issue coming up in the Images ecosystem for when you try to replace otsu_threshold ? What if we just define a find_threshold for ImageBinarization which essentially wraps the HistogramThresholding implementation?

@johnnychen94
Copy link
Member Author

johnnychen94 commented Jun 22, 2020

Is the name conflict issue coming up in the Images ecosystem for when you try to replace otsu_threshold?

Yes, it turns out that once we reexport any one of these two packages in Images.jl, this issue raises.

I'm not sure how to resolve the conflict of the AbstractImageBinarizationAlgorithm type which makes sense in ImageBinarization, and the AbstractHistogramThresholding type (currently just called ThresholdAlgorithm) which makes sense for HistogramThresholding.

I was thinking of this, too. Will try it and see what possible side effects would it be after this semester.

What if we just define a find_threshold for ImageBinarization which essentially wraps the HistogramThresholding implementation?

In that case, you're encouraging common users to not use HistogramThresholding anymore since find_threshold is the only function there, and if so, it would be better to merge HistogramThresholding as a sub package of ImageBinarization, as @timholy suggested in JuliaImages/Images.jl#898

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

Successfully merging a pull request may close this issue.

2 participants