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

chore: remove Graphics dependency #185

Merged
merged 1 commit into from
Jul 20, 2023
Merged

chore: remove Graphics dependency #185

merged 1 commit into from
Jul 20, 2023

Conversation

johnnychen94
Copy link
Member

It's unclear why this package is added to ImageCore so maybe we should clean up this dependency. I don't plan to merge this until our next breaking release, but it's worth raising this PR to get some feedback to ensure that these are really dead codes.

This, however, does not reduce the package loading latency because the loading time is mainly due to Colors:

julia> @time_imports using ImageCore
      0.6 ms  ┌ Compat
      0.8 ms  ┌ NaNMath
      0.6 ms    ┌ Adapt
     68.4 ms    ┌ OffsetArrays
     71.9 ms  ┌ PaddedViews
   1557.8 ms      ┌ SparseArrays 4.95% compilation time
   1564.1 ms    ┌ Statistics 5.20% compilation time
   1605.8 ms  ┌ FixedPointNumbers 5.06% compilation time
     77.9 ms    ┌ ChainRulesCore
     79.9 ms  ┌ ChangesOfVariables
      9.8 ms  ┌ AbstractFFTs
      1.4 ms  ┌ OpenLibm_jll
      2.6 ms    ┌ StackViews
      3.1 ms    ┌ MappedArrays
      7.7 ms  ┌ MosaicViews
      1.1 ms  ┌ InverseFunctions
      0.4 ms  ┌ Reexport
      4.4 ms  ┌ DocStringExtensions 64.64% compilation time
     73.9 ms    ┌ ColorTypes 7.20% compilation time
    187.3 ms    ┌ Colors
    265.1 ms  ┌ Graphics 2.01% compilation time
      4.9 ms  ┌ IrrationalConstants
      0.6 ms  ┌ TensorCore
      1.1 ms    ┌ LogExpFunctions
     22.5 ms        ┌ Preferences
     23.6 ms      ┌ JLLWrappers
    227.4 ms    ┌ OpenSpecFun_jll 88.92% compilation time (99% recompilation)
    274.0 ms  ┌ SpecialFunctions 73.79% compilation time (99% recompilation)
    141.0 ms  ┌ ColorVectorSpace 4.31% compilation time
   2867.8 ms  ImageCore 10.80% compilation time (65% recompilation)

@timholy My best guess is that ImageView needed it once to share the same function names width and height with Graphics. I've manually tested that ImageView works with this branch.

@SimonDanisch Does the Makie ecosystem require this?

@codecov
Copy link

codecov bot commented Jul 6, 2022

Codecov Report

Merging #185 (720b2a4) into master (6fc57c1) will decrease coverage by 0.19%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #185      +/-   ##
==========================================
- Coverage   62.67%   62.47%   -0.20%     
==========================================
  Files          10       10              
  Lines         584      581       -3     
==========================================
- Hits          366      363       -3     
  Misses        218      218              
Impacted Files Coverage Δ
src/ImageCore.jl 72.41% <ø> (ø)
src/traits.jl 93.33% <ø> (-0.22%) ⬇️

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 6fc57c1...720b2a4. Read the comment docs.

@SimonDanisch
Copy link
Member

No, Makie & friends have never relied on Graphics.jl ;)

@timholy
Copy link
Member

timholy commented Jan 5, 2023

Graphics.jl was intended as a generic interface for graphics operations but never got to the point of actually being useful.

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.

There may be some downstream failures from this, we should try to head those off quickly once this merges. But the version bump should insulate us from negative consequences.

@timholy timholy merged commit eadeca1 into master Jul 20, 2023
@timholy timholy deleted the jc/graphics branch July 20, 2023 15:00
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.

3 participants