Open oschulz opened 1 year ago
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
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).
I can do a PR for the DensityInterface support if you like?
That would be great! I'm traveling this week so my availability is limited.
No worries!
@oschulz @joshday still interested in this? Would make plotting a lot easier with ASH.
Sure. You had a PR but it needed some changes. Feel free to reopen it
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.
I'm definitely still interested.
Reopened!
+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 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.
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.