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

Deprecate eltype-specific convert methods #131

Merged
merged 2 commits into from
Aug 26, 2020
Merged

Conversation

timholy
Copy link
Member

@timholy timholy commented Jun 19, 2020

These are technically pirating and cause quite a few invalidations. Now that we have generic broadcasting they are no longer necessary. xref JuliaGraphics/ColorTypes.jl#196.

This first submission of the PR does not fix depwarns in the tests, because I want to see if everything passes before doing so.

Also xref #122. We should merge these two at around the same time, along with any other deprecations, to reduce the number of times we annoy users.

@codecov
Copy link

codecov bot commented Jun 19, 2020

Codecov Report

Merging #131 into master will increase coverage by 0.45%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #131      +/-   ##
==========================================
+ Coverage   60.50%   60.95%   +0.45%     
==========================================
  Files           9        9              
  Lines         400      397       -3     
==========================================
  Hits          242      242              
+ Misses        158      155       -3     
Impacted Files Coverage Δ
src/convert_reinterpret.jl 62.06% <ø> (-1.35%) ⬇️
src/deprecations.jl 47.05% <88.88%> (+47.05%) ⬆️

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 3beb222...f984124. Read the comment docs.

@dehann
Copy link

dehann commented Aug 10, 2020

+1 for this fix after coming across the same issue.

These are technically pirating and cause quite a few invalidations.
Now that we have generic broadcasting they are no longer necessary.
@timholy timholy force-pushed the teh/deprecate_convert branch from dc65d29 to f984124 Compare August 26, 2020 08:13
@timholy timholy merged commit 968e646 into master Aug 26, 2020
@timholy timholy deleted the teh/deprecate_convert branch August 26, 2020 09:07
@timholy
Copy link
Member Author

timholy commented Aug 26, 2020

I declare open season on making more disruptive changes.

Here's my plan:

  • I will get the rest of the Images ecosystem (or as much as I can discover) locally to the point where it's not triggering depwarns in internal code.
  • We release new patch-level versions of all the packages
  • We consider whether we need to signal to the users on Julia 1.5 and above. We can force depwarns to be printed like this:
const depwarnidx = (i = 1; T = Base.JLOptions; while i <= fieldcount(T) fieldname(T, i) === :depwarn && break; global i += 1; end; fieldoffset(T, i) + 1)
function forced_depwarn(msg, sym)
    oldval = unsafe_load(cglobal(:jl_options, UInt8), depwarnidx)
    unsafe_store!(cglobal(:jl_options, UInt8), Int8(true), depwarnidx)
    ret = try
        Base.depwarn(msg, sym)
    finally
        unsafe_store!(cglobal(:jl_options, UInt8), oldval, depwarnidx)
    end
    return ret
end

Personally I think we need to do that, given that most users probably say using Images and don't explicitly control the version of ImageCore they use.

@dehann
Copy link

dehann commented Aug 28, 2020

Great thanks for fixing this! Patch level updates sound like a good idea to me. Forcing depwarn printouts (or at least some kind of default=true) sounds like the right thing again. Type piracy can be tricky, so any guidance won't go amiss.

I'm a major fan of Images.jl btw, and likely to build a lot of application code using Images in the future.

@timholy
Copy link
Member Author

timholy commented Aug 28, 2020

Great to hear! Don't hesitate to advertise your work to us!

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.

2 participants