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

Adds deprecations for adjust_histogram(img, LinearStretching()) variants #51

Open
wants to merge 1 commit into
base: adjust_intensity
Choose a base branch
from

Conversation

zygmuntszpak
Copy link
Member

No description provided.

@zygmuntszpak zygmuntszpak changed the title depwarn("adjust_histogram!(img, LinearStretching()) is deprecated, use adjust_intensity!(img, LinearStretching()) instead", :adjust_histogram!) Adds deprecations for adjust_histogram(img, LinearStretching()) variants Sep 12, 2021
@codecov
Copy link

codecov bot commented Sep 12, 2021

Codecov Report

Merging #51 (77c3b48) into adjust_intensity (42795dd) will increase coverage by 0.02%.
The diff coverage is 29.41%.

Impacted file tree graph

@@                 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     
Impacted Files Coverage Δ
src/algorithms/linear_stretching.jl 95.00% <ø> (ø)
src/deprecations.jl 29.41% <29.41%> (ø)
src/ContrastAdjustmentAPI/intensity_adjustment.jl 42.85% <0.00%> (+42.85%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42795dd...77c3b48. Read the comment docs.

Comment on lines +3 to +9
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
Copy link
Member

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

Suggested change
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...)

Copy link
Member Author

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?

Copy link
Member

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...)

@timholy
Copy link
Member

timholy commented Sep 15, 2021

Is there any reason not to do a wholesale rename adjust_histogram -> adjust_intensity? Don't all of our adjustment methods operate solely on the intensity? For RGB we convert to YIQ and then adjust just Y, so that too modifies only the intensity: the term adjust_histogram is a bit ambiguous about how we handle RGB (it leaves open the possibility that we operate channelwise), but the term adjust_intensity is quite clear.

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.

@zygmuntszpak
Copy link
Member Author

I'm OK with renaming it all as adjust_intensity as well. Is there a case to be made for adjust_intensities ?

@timholy
Copy link
Member

timholy commented Sep 15, 2021

That sounds fine too, and perhaps even better.

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 this pull request may close these issues.

None yet

3 participants