JuliaImages / DitherPunk.jl

Dithering algorithms in Julia.
MIT License
60 stars 3 forks source link

lazily load feature packages #77

Closed johnnychen94 closed 2 years ago

johnnychen94 commented 2 years ago

Loading time is almost the same:

julia> @time using DitherPunk
Before:   0.821818 seconds (2.66 M allocations: 204.827 MiB, 6.21% gc time, 17.57% compilation time: 84% of which was recompilation)
After:  0.901982 seconds (2.74 M allocations: 211.186 MiB, 8.22% gc time, 15.39% compilation time: 90% of which was recompilation)

The benefits are that users don't need to battle with conditional dependencies.

using TestImages
using DitherPunk
img = testimage("cameraman")

Before:

julia> @time braille(img, FloydSteinberg());
ERROR: UndefVarError: braille not defined
Stacktrace:
 [1] top-level scope
   @ ./timing.jl:267 [inlined]
 [2] top-level scope
   @ ./REPL[4]:0

julia> using UnicodePlots
 │ Package UnicodePlots not found, but a package named UnicodePlots is available from a registry. 
 │ Install package?
 │   (DitherPunk) pkg> add UnicodePlots 
 └ (y/n/o) [y]:
...

julia> @time using UnicodePlots
  1.481051 seconds (5.44 M allocations: 393.949 MiB, 5.50% gc time, 29.23% compilation time: 72% of which was recompilation)

julia> @time braille(img, FloydSteinberg());
  1.196503 seconds (4.25 M allocations: 289.933 MiB, 9.40% gc time, 99.22% compilation time)

julia> @time braille(img, FloydSteinberg());
  0.008986 seconds (105 allocations: 11.138 MiB)

After:

julia> @time braille(img, FloydSteinberg()); # The UnicodePlots loading is delayed here
  2.311043 seconds (9.29 M allocations: 655.996 MiB, 5.64% gc time, 53.31% compilation time)

julia> @time braille(img, FloydSteinberg());
  0.319657 seconds (556.66 k allocations: 48.261 MiB, 5.55% gc time, 97.48% compilation time: 100% of which was recompilation)

julia> @time braille(img, FloydSteinberg()); # LazyModules adds about ~80ns overhead, which is not an issue for any non-trivial function.
  0.009811 seconds (110 allocations: 11.138 MiB)

@adrhill can you help update the documentation accordingly?

codecov[bot] commented 2 years ago

Codecov Report

Merging #77 (37d5ba9) into master (6e6ac5e) will decrease coverage by 0.05%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #77      +/-   ##
==========================================
- Coverage   97.16%   97.10%   -0.06%     
==========================================
  Files          14       13       -1     
  Lines         247      242       -5     
==========================================
- Hits          240      235       -5     
  Misses          7        7              
Impacted Files Coverage Δ
src/api/color.jl 90.00% <ø> (ø)
src/clustering.jl 100.00% <ø> (ø)
src/DitherPunk.jl

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 6e6ac5e...37d5ba9. Read the comment docs.

adrhill commented 2 years ago

@johnnychen94 there are still some issues with Julia 1.0:

LoadError: iteration is deliberately unsupported for CartesianIndex. Use `I` rather than `I...`, or use `Tuple(I)...`

caused by src/error_diffusion.jl:57:

CI = CartesianIndices(filter) .- CartesianIndex(1, offset)

Should I replace this by something like

CI = [CartesianIndex(I .- (1, offset)) for I in CartesianIndices(filter)]

or is there a more elegant way?

adrhill commented 2 years ago

Thanks for all the benchmarks!

johnnychen94 commented 2 years ago

I see. That's probably the only way to work around (or one could probably use Compat.jl) the compatibility issue.

Maintaining 1.0 compatibility becomes more and more troublesome these days because many dependencies (e.g., UnicodePlots, ArrayInterface) require >=1.6. I propose to save ourselves some effort and just set the Julia lower bound to 1.6. How do you think?

adrhill commented 2 years ago

Sounds good to me. I'll merge this and remove 1.0 in a separate PR to make it visible in the commit history.