AverageShiftedHistograms.jl icon indicating copy to clipboard operation
AverageShiftedHistograms.jl copied to clipboard

Removing UnicodePlots as a dependency to reduce package load time?

Open oschulz opened this issue 2 years ago • 12 comments
trafficstars

I'd love to use AverageShiftedHistograms for density estimation in BAT.jl (density estimation for marginals) and other places, but the dependency on UnicodePlots makes AverageShiftedHistograms very heavy. UnicodePlots is a monster with a load time of about 3 seconds, whereas AverageShiftedHistograms itself is very lightweight, and it's other dependencies are not very heavy either.

@joshday how would you feel about removing UnicodePlots as a dependency? Users would have to call plot explicitly then, of course, but it would likely reduce the load time of AverageShiftedHistograms drastically.

oschulz avatar Dec 05 '22 11:12 oschulz

Yeah, I think I'm okay with that, although I do like the instant feedback of the density in the show methods.

Would supporting DensityInterface be sufficient for your use case? See https://github.com/joshday/AverageShiftedHistograms.jl/pull/40

joshday avatar Dec 05 '22 13:12 joshday

Thanks Josh - yes, I was actually about to suggest supporting DensityInterface, I'd overlooked #40 :-)

DensityInterface is very lightweight by design, and it's densityof and logdensityof can replace the custom PDF-functions in AverageShiftedHistograms. I think we should have DensityKind(::Ash) = HasDensity(), since an Ash is semantically actually a probability measure/distribution.

DensityInterface currently doesn't offer a CDF-Function, but we may have a lightweigt DistributionsBase in the future (JuliaStats/Distributions.jl#1139).

oschulz avatar Dec 05 '22 16:12 oschulz

I can do a PR for the DensityInterface support if you like?

oschulz avatar Dec 05 '22 16:12 oschulz

That would be great! I'm traveling this week so my availability is limited.

joshday avatar Dec 05 '22 20:12 joshday

No worries!

oschulz avatar Dec 06 '22 01:12 oschulz

@oschulz @joshday still interested in this? Would make plotting a lot easier with ASH.

ParadaCarleton avatar Jun 21 '23 18:06 ParadaCarleton

Sure. You had a PR but it needed some changes. Feel free to reopen it

joshday avatar Jun 21 '23 19:06 joshday

Oh, that's why! I could've sworn I'd made a PR for this at some point. Can you reopen it? I don't have permission.

ParadaCarleton avatar Jun 21 '23 19:06 ParadaCarleton

I'm definitely still interested.

oschulz avatar Jun 21 '23 19:06 oschulz

Reopened!

joshday avatar Jun 21 '23 21:06 joshday

+1 for this. On Julia v1.10.0-beta1, this package for me takes 0.66s to load with UnicodePlots and 0.16s without UnicodePlots. As a dependency, that 0.16s is probably nothing, since most packages that would add this as a dep probably also (in)directly depend on the rest of ASH's dependencies.

You could always do something like this

const SHOW_UNICODE_PLOTS = Ref(false)

function show_unicode_plots(tf::Bool)
    SHOW_UNICODE_PLOTS[] = tf
end

function Base.show(io::IO, ::MIME"text/plain", o::Ash)
    ...
    if SHOW_UNICODE_PLOTS[]
        _show_unicode_plot(io, o)
    end
end

function _show_unicode_plot end

And then in an AverageShiftedHistogramsUnicodePlotsExt extension overload

AverageShiftedHistograms._show_unicode_plot(io::IO, o::Ash) = ...

Then if a user really wants the nice REPL output, they can load UnicodePlots themselves and enable it manually.

sethaxen avatar Aug 11 '23 13:08 sethaxen

@sethaxen I'd be cool with a package extensions implementation. I won't be able to work on it for a while, but I would accept a PR.

joshday avatar Aug 11 '23 14:08 joshday