Introduces AbstractIntensityAdjustmentAlgorithm #50
Conversation
Codecov Report
@@ Coverage Diff @@
## master #50 +/- ##
==========================================
- Coverage 95.97% 92.12% -3.85%
==========================================
Files 12 13 +1
Lines 546 597 +51
==========================================
+ Hits 524 550 +26
- Misses 22 47 +25
Continue to review full report at Codecov.
|
| For more examples, please check [`adjust_intensity`](@ref), | ||
| [`adjust_intensity!`](@ref) and concrete algorithms. | ||
| """ | ||
| abstract type AbstractIntensityAdjustmentAlgorithm <: AbstractImageFilter end |
There was a problem hiding this comment.
Another choice is to make it
abstract type AbstractIntensityAdjustmentAlgorithm <: AbstractHistogramAdjustmentAlgorithm endand so that we don't need to deprecate the old usages. We can just incrementally add new adjust_intensity methods with default alg to be LinearStretching. This means that both adjust_histogram and adjust_intensity would work for AbstractIntensityAdjustmentAlgorithm types.
How do you think?
There was a problem hiding this comment.
I think that making
abstract type AbstractIntensityAdjustmentAlgorithm <: AbstractHistogramAdjustmentAlgorithm endgoes against the spirit of the issue #32 which seems to be specifically about the fact that operations that don't manipulate the histogram ought not to be conceptualised as histogram adjustment algorithms.
I reckon let's just go ahead and make the deprecations before we embed this all in the Images ecosystem.
There was a problem hiding this comment.
goes against the spirit of the issue #32 which seems to be specifically about the fact that operations that don't manipulate the histogram ought
My argument is that not all histogram adjustment algorithm needs to explicitly construct a histogram. Directly adjust intensity can be considered as an implicit way to scale the x scale of the intensity histogram.
My personal understanding of #32 is that we just need a default algorithm for easier and more convenient usage to close #32. For generic histogram adjustment, there isn't a consensus default algorithm, but for intensity adjustment, we can default to linear stretching.
There was a problem hiding this comment.
My argument is that not all histogram adjustment algorithm needs to explicitly construct a histogram. Directly adjust intensity can be considered as an implicit way to scale the x scale of the intensity histogram.
That was also my argument and the reason why I implemented it the way I did in the first place :D, but @stillyslalom seems to disagree with our view here.
We can just incrementally add new adjust_intensity methods with default alg to be LinearStretching. This means that both adjust_histogram and adjust_intensity would work for AbstractIntensityAdjustmentAlgorithm types.
I'm torn here. If we go this route, then what's the benefit of introducing the adjust_intensity function? Is it effectively not going to just be an alias for adjust_histogram?
I guess one distinction is that AbstractIntensityAdjustmentAlgorithm is typically embarrassingly parallelisable, whereas some of the histogram-based algorithms may not be.
My personal understanding of #32 is that we just need a default algorithm for easier and more convenient usage to close #32. For generic histogram adjustment, there isn't a consensus default algorithm, but for intensity adjustment, we can default to linear stretching.
I was planning to introduce some convenience functions such as span_contrast to address issues such as #27
There was a problem hiding this comment.
I think all we need here is to introduce a convenient function span_contrast backed by LinearStretching to save some typings and easier to remember without checking the documentation. Changing the type hierarchy doesn't make things easier.
In the meantime, we can generalize LinearStretching and solves #33
There was a problem hiding this comment.
My argument is that not all histogram adjustment algorithm needs to explicitly construct a histogram. Directly adjust intensity can be considered as an implicit way to scale the x scale of the intensity histogram.
I don't know if I agree with this argument. We don't call fill! adjust_variance even though it does that.
There was a problem hiding this comment.
Thanks for sharing your thoughts. What is your suggestion regarding the type hierarchy? Should AbstractIntensityAdjustmentAlgorithn subtype the AbstractHistogramAdjustmentAlgorithm type?
There was a problem hiding this comment.
Vice versa, I'd say. The second sounds way more specific, the first is much more vague.
This is a work in progress to address #32