-
Notifications
You must be signed in to change notification settings - Fork 5
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
Introduces AbstractIntensityAdjustmentAlgorithm #50
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another choice is to make it
abstract type AbstractIntensityAdjustmentAlgorithm <: AbstractHistogramAdjustmentAlgorithm end
and 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that making
abstract type AbstractIntensityAdjustmentAlgorithm <: AbstractHistogramAdjustmentAlgorithm end
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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