search

JuliaStats / KernelDensity.jl

Kernel density estimators for Julia
Other
172 stars 40 forks source link

Remove optim in favor of independent golden section implementation #84

Closed piever closed 3 years ago

piever commented 4 years ago

Fix #83. This adds a simple implementation of the golden section algorithm, so that KernelDensity can drop the Optim dependency. The issue (at least for me) is that Optim is somewhat heavy (3 seconds of load time), and KernelDensity is needed as a dependency for plotting packages for 1D and 2D density plots, so ideally it should be a bit more lightweight.

Removing Optim shaves off a couple of seconds from load time.

On master:

julia> @time using KernelDensity
  6.803513 seconds (12.15 M allocations: 706.212 MiB, 2.90% gc time)

With this PR:

julia> @time using KernelDensity
  4.620525 seconds (7.87 M allocations: 475.978 MiB, 0.72% gc time)
piever commented 4 years ago

Bump?

tpapp commented 4 years ago

This looks ok to me at a first glance but I haven't had time for a detailed review, sorry. Will try to do it soon.

piever commented 3 years ago

Sorry to bump this again, but it seems that it is important to get this in to move forward with changes in Makie. We are adding support to statistical plots in AbstractPlotting.jl (see https://github.com/JuliaPlots/AbstractPlotting.jl/pull/478), and it looks like this PR mitigates the increase in load time substantially.

Here are times to load (after a first run to precompile) on julia 1.5

# AbstractPlotting master 
julia> @time using AbstractPlotting
  6.322670 seconds (9.76 M allocations: 600.265 MiB, 1.30% gc time)

# AbstractPlotting with pv/stats PR and KernelDensity release
julia> @time using AbstractPlotting
 13.004530 seconds (19.41 M allocations: 1.097 GiB, 3.11% gc time)

# AbstractPlotting with pv/stats PR and this PR on KernelDensity
julia> @time using AbstractPlotting
 10.147386 seconds (15.28 M allocations: 905.327 MiB, 2.54% gc time)

(Removing Optim seems like the only easy way to speed things up, because it is not used by most other statistical packages, whereas most other deps are shared and would be much harder to remove.)

tpapp commented 3 years ago

Sorry for not reviewing earlier, I appreciate the ping. I requested some minor changes, and will merge ASAP when this converges.

piever commented 3 years ago

Thank you for the review! I've addressed the feedback.

I requested some minor changes, and will merge ASAP when this converges.

Great! Could it be possible to also have a release after this is merged?