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 deprecation warnings and exports ImageContrastAdjustment #865

Merged
merged 1 commit into from
Jan 29, 2020

Conversation

zygmuntszpak
Copy link
Member

Also removes code in exposure.jl which is now found in the ImageContrastAdjustment.jl package.

@zygmuntszpak
Copy link
Member Author

Addresses part of #841

@zygmuntszpak
Copy link
Member Author

Are we OK to bump the minimum requirement for Julia for the Images package to version 1.1? The minimum requirement for ImageContrastAdjustment is Julia 1.1 and I now recall that this is because I used Base.@kwdef in various place. That function only got introduced in Julia version 1.1 https://pkg.julialang.org/docs/julia/THl1k/1.1.1/NEWS.html

@Tokazama
Copy link

I personally think that if JuliaPro has reached a version number for a while it's probably safe to set it as the minimum version number. They seem to be pretty proactive about making sure packages work with each new JuliaPro release.

@johnnychen94
Copy link
Member

Unless there's another LTS version, dropping 1.0 compatibility is really risking

@Tokazama
Copy link

I guess we should probably consider which "Risk Tolerance Persona" we should cater to.

@timholy
Copy link
Member

timholy commented Jan 23, 2020

Yeah, I'm a bit uncomfortable about not continuing to support Julia 1.0. How bad would it be to just write out the default-filling constructors manually?

Otherwise, this is really nice to see!

@zygmuntszpak
Copy link
Member Author

I've tagged a new version of ImageContrastAdjustment which now supports Julia 1.0. Hence, I am going to close and re-open this pull-request to trigger a new build.

@zygmuntszpak
Copy link
Member Author

@timholy I'll wait for you to approve this before I merge.

Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main issue is I think your depwarns don't actually work. If you tested them and I'm wrong about that, please let me know.

src/exposure.jl Outdated Show resolved Hide resolved
@zygmuntszpak zygmuntszpak force-pushed the contrast branch 2 times, most recently from fe98ffa to 6f186dc Compare January 26, 2020 07:51
Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Just a couple of changes below.

Eventually, we'll just delete test/exposure.jl, right? If yes, maybe add a comment at the top of the file saying it can be removed when src/deprecations.jl is deleted. If no, we might want to separate out the stuff we'll keep and move the stuff we'll delete into test/deprecations.jl.

src/deprecations.jl Outdated Show resolved Hide resolved
src/Images.jl Show resolved Hide resolved
src/deprecations.jl Outdated Show resolved Hide resolved
src/deprecations.jl Outdated Show resolved Hide resolved
zygmuntszpak added a commit to JuliaImages/ImageContrastAdjustment.jl that referenced this pull request Jan 27, 2020
The change made in 0.3.2 still triggers a warning message. See: JuliaImages/Images.jl#865 (comment)
Also removes code in exposure.jl which is now found in the ImageContrastAdjustment.jl package.
@zygmuntszpak
Copy link
Member Author

  • Moved all deprecated code from exposure.jl into deprecations.jl
  • Deprecation messages say "is deprecated" instead of "will be deprecated".
  • Removed mention of adjust_histogram, adjust_histogram!, Matching, Equalization and build_histogram from export statement (it is now automatically reexported)
  • Rerouted old variants of adjust_histogram, adjust_histogram!, and build_histogram so that they call the new API in ImageContrastAdjustment. User receives a deprecation message, but their code still works (with the exception of code that used Matching()).
  • Any code that matched histograms using adjust_histogram(Matching(), ...) is broken. The user receives an error message stating that the keyword targetimg in the construction of Matching needs to be supplied.

@johnnychen94
Copy link
Member

It's the Spring festival here in China so I didn't follow the whole discussion here with my limited time. Looks like there're still some breaking changes, so a minor version bump is required, i.e., 0.21.0

Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's give anyone else who wants to review another day to check it, and then merge.

@zygmuntszpak zygmuntszpak merged commit 45b0c2d into master Jan 29, 2020
@zygmuntszpak zygmuntszpak deleted the contrast branch January 29, 2020 01:13
@timholy
Copy link
Member

timholy commented Jan 30, 2020

Thanks for merging, I've been swamped! We should get this out as 0.21, will try to get to it over the weekend.

@timholy
Copy link
Member

timholy commented Feb 4, 2020

And it's out! Thanks again @zygmuntszpak, great contribution!

@tlnagy
Copy link
Contributor

tlnagy commented Feb 7, 2020

The suggested fix in the dep warning for imadjustintensity doesn't work:

julia> using Images

julia> img = Gray{N0f8}.(rand(10, 10));

julia> imadjustintensity(img);
┌ Warning: `imadjustintensity` will be removed in a future release, please use `adjust_histogram(img, LinearStretching())` instead.
│   caller = top-level scope at REPL[10]:1
└ @ Core REPL[10]:1

julia> adjust_histogram(img, LinearStretching())
ERROR: MethodError: no method matching adjust_histogram(::Array{Gray{Normed{UInt8,8}},2}, ::LinearStretching{Float64,Float64})
Stacktrace:
 [1] top-level scope at REPL[11]:1

@zygmuntszpak
Copy link
Member Author

It looks like the reason for this is that I forgot to add

import ImageContrastAdjustment: adjust_histogram, adjust_histogram!

to ensure that the new definitions of adjust_histogram and adjust_histogram! in deprecations.jl don't make the definitions in ImageContrastAdjustment "disappear".

I'm putting together a quick fix, and will add some test to make sure that the advertised API is actually callable. Sorry for the oversight, and thanks for reporting the issue.

@timholy
Copy link
Member

timholy commented Jul 23, 2023

Another thing we should have done is fill in the parameters for the actual usage; anyone who copy/pastes the suggested deprecation would be throwing away whatever custom parameters they're supplying for the call. A prototype of a better deprecation might have been

@deprecate clahe(img, nbins=100) adjust_histogram(img, AdaptiveEqualization(; nbins))

and then the user will see a copy/pastable suggestion.

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

6 participants