-
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
Adds deprecations for adjust_histogram(img, LinearStretching()) variants #51
base: adjust_intensity
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## adjust_intensity #51 +/- ##
====================================================
+ Coverage 92.12% 92.15% +0.02%
====================================================
Files 13 14 +1
Lines 597 612 +15
====================================================
+ Hits 550 564 +14
- Misses 47 48 +1
Continue to review full report at Codecov.
|
426e964
to
77c3b48
Compare
function adjust_histogram(img::Union{GenericGrayImage, AbstractArray{<:Color3}}, | ||
f::LinearStretching, | ||
args...; kwargs...) | ||
|
||
depwarn("adjust_histogram(img, LinearStretching()) is deprecated, use adjust_intensity(img, LinearStretching()) instead", :adjust_histogram) | ||
return adjust_intensity(img, f, args...;kwargs...) | ||
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.
For these I think we can simplify it with
function adjust_histogram(img::Union{GenericGrayImage, AbstractArray{<:Color3}}, | |
f::LinearStretching, | |
args...; kwargs...) | |
depwarn("adjust_histogram(img, LinearStretching()) is deprecated, use adjust_intensity(img, LinearStretching()) instead", :adjust_histogram) | |
return adjust_intensity(img, f, args...;kwargs...) | |
end | |
@deprecate adjust_histogram(img::AbstractArray, f::LinearStretching, args...; kwargs...) adjust_intensity(img, f, args...; kwargs...) |
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.
It turns out that writing the longer variant is useful because I can then unify all the deprecation messages for the 3 types that need to change (LinearStretching, GammaCorrection and ContrastStretching) into one as follows:
function adjust_histogram!(img::Union{GenericGrayImage, AbstractArray{<:Color3}},
f::Union{LinearStretching, GammaCorrection, ContrastStretching},
args...; kwargs...)
algo = typeof(f)
depwarn("adjust_histogram!(img, $algo) is deprecated, use adjust_intensity!(img, $algo) instead", :adjust_histogram!)
return adjust_intensity!(img, f, args...; kwargs...)
end
That should save us from a bunch of code duplication. What 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.
It's just that in this case we don't need to manually construct the depwarn
message info:
const DeprecatedIntensityMethods = Union{LinearStretching, GammaCorrection, ContrastStretching}
@deprecate adjust_histogram(img::AbstractArray, f::DeprecatedIntensityMethods, args...; kwargs...) adjust_intensity(img, f, args...; kwargs...)
Is there any reason not to do a wholesale rename It would be wrong if we did something other than modifying the pixelwise intensity, so if there are such places then please do bring them up. |
I'm OK with renaming it all as |
That sounds fine too, and perhaps even better. |
No description provided.