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

Removing UnicodePlots as a dependency to reduce package load time? #41

Open
oschulz opened this issue Dec 5, 2022 · 12 comments
Open

Removing UnicodePlots as a dependency to reduce package load time? #41

oschulz opened this issue Dec 5, 2022 · 12 comments

Comments

@oschulz
Copy link

oschulz commented Dec 5, 2022

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.

@joshday
Copy link
Owner

joshday commented Dec 5, 2022

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 #40

@oschulz
Copy link
Author

oschulz commented Dec 5, 2022

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
Copy link
Author

oschulz commented Dec 5, 2022

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

@joshday
Copy link
Owner

joshday commented Dec 5, 2022

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

@oschulz
Copy link
Author

oschulz commented Dec 6, 2022

No worries!

@ParadaCarleton
Copy link
Contributor

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

@joshday
Copy link
Owner

joshday commented Jun 21, 2023

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

@ParadaCarleton
Copy link
Contributor

ParadaCarleton commented Jun 21, 2023

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.

@oschulz
Copy link
Author

oschulz commented Jun 21, 2023

I'm definitely still interested.

@joshday
Copy link
Owner

joshday commented Jun 21, 2023

Reopened!

@sethaxen
Copy link

+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.

@joshday
Copy link
Owner

joshday commented Aug 11, 2023

@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.

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

No branches or pull requests

4 participants