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

conditionally use SpecialFunctions through weak deps or Requires #186

Merged
merged 4 commits into from
Feb 7, 2023

Conversation

t-bltg
Copy link
Contributor

@t-bltg t-bltg commented Feb 4, 2023

Fix #184.
Fix #182.
Fix last part of JuliaPlots/UnicodePlots.jl#291.

This is a proposal to make all the SpecialFunctions related functions (which are clearly piracy) optional using weak dependencies on julia >= 1.9 and Requires on older releases.

This would be (slightly) breaking so I guess we should bump ColorVectorSpace to 0.10.0 if this PR is merged, since you'd have to use:

julia> using SpecialFunctions
julia> using ColorVectorSpace

to recover all the functionalities on current master.

Note that lfact is renamed to logfactorial since lfact was deprecated in SpecialFunctions.

I followed the guidelines in https://pkgdocs.julialang.org/dev/creating-packages/#Conditional-loading-of-code-in-packages-(Extensions) and https://docs.julialang.org/en/v1.10-dev/manual/code-loading/#man-extensions.

This PR was written for ColorSchemes, and pkg> test ColorSchemes passes locally since ColorSchemes does not depend on the SpecialFunctions functionalities defined in ColorVectorSpace.

Latency improvements:

PR

julia> @time @time_imports using ColorVectorSpace
      1.0 ms  Statistics
     66.4 ms  FixedPointNumbers
    100.7 ms  ColorTypes
      0.4 ms  TensorCore
    319.3 ms  ColorVectorSpace
  0.613890 seconds (553.44 k allocations: 39.516 MiB, 17.56% gc time, 16.28% compilation time)

master

julia> @time @time_imports using ColorVectorSpace
      1.5 ms  Statistics
     85.7 ms  FixedPointNumbers
    124.7 ms  ColorTypes
      8.0 ms  IrrationalConstants
      0.2 ms  Compat
    105.7 ms  ChainRulesCore
      1.4 ms  DocStringExtensions
      0.4 ms  LogExpFunctions
      0.3 ms  OpenLibm_jll
     23.2 ms  Preferences
      0.3 ms  JLLWrappers
      7.0 ms  OpenSpecFun_jll 90.69% compilation time
     22.2 ms  SpecialFunctions
      0.3 ms  TensorCore
    203.7 ms  ColorVectorSpace
  0.642129 seconds (628.22 k allocations: 43.283 MiB, 5.46% compilation time)

Ping @timholy or @kimikage or @johnnychen94 for review (primary contributors to this repo).

@t-bltg t-bltg changed the title weak deps conitionally use SpecialFunctions through weak deps or Requires Feb 4, 2023
@t-bltg t-bltg changed the title conitionally use SpecialFunctions through weak deps or Requires conditionally use SpecialFunctions through weak deps or Requires Feb 4, 2023
@codecov
Copy link

codecov bot commented Feb 4, 2023

Codecov Report

Base: 86.41% // Head: 91.44% // Increases project coverage by +5.03% 🎉

Coverage data is based on head (a48c42c) compared to base (8ec1d35).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #186      +/-   ##
==========================================
+ Coverage   86.41%   91.44%   +5.03%     
==========================================
  Files           2        3       +1     
  Lines         265      269       +4     
==========================================
+ Hits          229      246      +17     
+ Misses         36       23      -13     
Impacted Files Coverage Δ
ext/SpecialFunctionsExt.jl 100.00% <100.00%> (ø)
src/ColorVectorSpace.jl 90.87% <100.00%> (ø)
src/precompile.jl 100.00% <0.00%> (+100.00%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

Deep review comments 😆 below. I think this is a good idea. We need to make a breaking release soon anyway because we are way behind schedule on https://github.com/JuliaGraphics/ColorVectorSpace.jl#abs-and-abs2.

src/ColorVectorSpace.jl Outdated Show resolved Hide resolved
@t-bltg t-bltg mentioned this pull request Feb 4, 2023
@timholy timholy merged commit 61e78ec into JuliaGraphics:master Feb 7, 2023
@timholy
Copy link
Member

timholy commented Feb 7, 2023

Thanks so much, @t-bltg!

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.

Remove SpecialFunctions dep (or replace with Bessels) conditionally require SpecialFunctions
3 participants