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

Dropping Requires-based extensions for Distributions, SpecialFunctions and StatsFuns? #11

Closed
oschulz opened this issue May 8, 2022 · 29 comments

Comments

@oschulz
Copy link

oschulz commented May 8, 2022

The @requires for Distributions, SpecialFunctions and StatsFuns turn LogarithmicNumbers from a lightweight package into an effectively quite heavy package, increasing the load time of packages depending on it (like @cscherrer's MeasureBase) to a degree that makes them unattractive as dependencies t themselves (I'd like to use MeasureBase.jl in BATBase.jl, for example, as part of bat/BAT.jl#351, but currently it's a too heavy because of LogarithmicNumbers' requires`).

It looks like Distributions & friends will not support LogarithmicNumbers directly (JuliaStats/Distributions.jl#1545):

@devmotion: I think it would be simpler if users that want to wrap the result of logpdf, logcdf etc. computations as a ULogarithmic use explicit calls of e.g. uexp(logpdf(dist, x)) etc. Load times could be improved for users of Distributions + LogarithmicNumbers by removing the overloads in LogarithmicNumbers and dropping the Requires dependency there.

@cjdoris, Would you consider dropping the @requires, as suggested by @devmotion?

@gdalle
Copy link
Contributor

gdalle commented May 8, 2022

@cjdoris maybe a solution would be to switch to the lighter https://github.com/JuliaMath/DensityInterface.jl ?

@devmotion
Copy link

devmotion commented May 8, 2022

That would still lead to a LogarithmicNumber-specific API that is inconsistent with the original API in DensityInterface. IMO it seems also easier to just let users call uexp(logdensityof(...)) instead of something like densityof(ULogFloat64, ...).

@oschulz
Copy link
Author

oschulz commented May 8, 2022

let users call uexp(logdensityof(...)) instead of something like logdensityof(ULogFloat64, ...)

I agree - it removes dependencies, is clear to read and just as compact.

@gdalle
Copy link
Contributor

gdalle commented May 8, 2022

On the other hand I like the fact that logdensityof a logarithmic number works out of the box. This allowed me to implement a log-scale HMM estimation algorithm without changing anything to my code, simply by allowing LogarithmicNumbers as input

@devmotion
Copy link

devmotion commented May 8, 2022

AFAICT this is different from the issue here. If logpdf etc. is implemented generically enough in Distributions (which it should), calling logpdf(d, x) where x and/or the parameters of d are logarithmic numbers should just work. It should not be necessary for one package to know about the other. The current overloads discussed in this issue are of the form pdf(::Type{<:Logarithmic}, ...) and don't seem needed for this use case.

@gdalle
Copy link
Contributor

gdalle commented May 8, 2022

Alright, my bad :)

@oschulz
Copy link
Author

oschulz commented May 8, 2022

I like the fact that logdensityof a logarithmic number works out of the box

You don't mean logdensityof(logarithmic_number), right? Because that shouldn't work, logdensityof is supposed to take objects that are or have densities as it's argument, not actual density values. If you already have the number you would do log(logarithmic_number), if I understand correctly.

@gdalle
Copy link
Contributor

gdalle commented May 8, 2022

Sure, I meant stuff like logdensityof(distribution, array_of_lognumbers)

@oschulz
Copy link
Author

oschulz commented May 8, 2022

Sure, I meant stuff like logdensityof(distribution, array_of_lognumbers)

Oh, right, of course!

@cscherrer
Copy link

cscherrer commented May 8, 2022

Thanks @oschulz , it would be great to have this package loading faster!

For some additional context, the core of the dependencies seems to be this:

const DISTRIBUTIONS_OVERLOADS = [f => Symbol(:log, f) for f in [:cdf, :ccdf, :pdf]]
const STATSFUNS_DISTS = [:srdist, :nchisq, :hyper, :ntdist, :tdist, :binom, :pois, :fdist, :norm, :beta, :nfdist, :chisq, :gamma, :nbeta, :nbinom]
const STATSFUNS_OVERLOADS = [Symbol(d, f) => Symbol(d, :log, f) for d in STATSFUNS_DISTS for f in [:cdf, :ccdf, :pdf]]
const SPECIALFUNCTIONS_OVERLOADS = [f => Symbol(:log, f) for f in [:gamma, :factorial, :beta, :erfc, :erfcx]]

This maps each function name f to a name for log ∘ f. I haven't measured, but I think StatsFuns and SpecialFunctions are pretty light-weight. And there definitely common enough that many people using LogarithmicNumbers would be using them already anyway.

So one possibility would be for this package to depend explicitly on StatsFuns and SpecialFunctions. But if the goal is to get rid of the @requires and Distributions will not add the dependency, we'd lose the DISTRIBUTION_OVERLOADS, which would be unfortunate.

One possibility of avoiding this would even allow this package to ignore StatsFuns and SpecialFunctions, and things could still "just work". Maybe there could be a very lightweight interface package [yet again, these just seem to solve everything] specifying higher-order functions f -> log ∘ f and f -> exp ∘ f. That would give any package a way to give semantic context relating these functions, similar to the approach in InverseFunctions.jl.

For example, if these are called logcirc and expcirc (just placeholder names, not suggesting these), then Distributions could depend on this proposed very light package and then add

logcirc(::typeof(pdf)) = logpdf
logcirc(::typeof(cdf)) = logcdf
logcirc(::typeof(ccdf)) = logccdf
expcirc(::typeof(logpdf)) = pdf
expcirc(::typeof(logcdf)) = cdf
expcirc(::typeof(logccdf)) = ccdf

@oschulz
Copy link
Author

oschulz commented May 8, 2022

But if the goal is to get rid of the @requires and Distributions will not add the dependency, we'd lose the DISTRIBUTION_OVERLOADS, which would be unfortunate.

But why would that be so unfortunate? From what I understand, we'd only lose convenience definitions like pdf(ULogarithmic, d, x) - I think @devmotion makes a good point here, writing uexp(logpdf(d, x)) is not less compact and very clear.

@oschulz
Copy link
Author

oschulz commented May 8, 2022

I haven't measured, but I think StatsFuns and SpecialFunctions are pretty light-weight. [...] So one possibility would be for this package to depend explicitly on StatsFuns and SpecialFunctions [...]

They are not light-weight, really - compare

julia> using InverseFunctions # Load a super-lightweight package to get some initial Pkg costs out of the way

julia> @time_imports using SpecialFunctions, StatsFuns, Distributions, LogarithmicNumbers, MeasureBase
    [...]
    226.6 ms  SpecialFunctions
    [...]
     92.7 ms  StatsFuns
    [...]
    470.4 ms  Distributions
    [...]
    379.7 ms  LogarithmicNumbers
    [...]
     74.8 ms  MeasureBase

with

julia> using InverseFunctions

julia> @time_imports using LogarithmicNumbers, MeasureBase
      0.4 ms  ┌ Requires
     37.1 ms  LogarithmicNumbers
    [...]
    287.2 ms  MeasureBase

So the load time of LogarithmicNumbers without requires is just about 40 ms, and the cost of MeasureBase on top of LogarithmicNumbers and it's other deps would just be 74.8. SpecialFunctions and Distributions are much heavier, and StatsFuns isn't exactly lightweight either - timed relatively on top of each other.

If LogarithmicNumbers were to depend on SpecialFunctions and StatsFuns, it's load time would be over 300 ms, while without @requires it can load in under 40 ms.

These times may not seem long, but larger use cases have many dependencies that add up, and suddenly you end up with load times of 10 seconds for a use case (and then I have to listen to people telling me "but in Python it would already be finished. :-) ).

@cjdoris
Copy link
Owner

cjdoris commented May 8, 2022

Having read this discussion and the one over on Distributions, I think I agree that just removing all those overloads is the right thing to do. No registered packages use the behaviour.

@cscherrer
Copy link

cscherrer commented May 8, 2022

This is a little off topic, but I'm generally opposed to making real design decisions for the benefit of people who think using JIT compilation for a short-running script is a good idea. That use case can benefit from static compiler work, otherwise it's a terrible cost model.

writing uexp(logpdf(d, x)) is not less compact and very clear.

Good point, I had missed this. So, maybe the requires-es can just go away? Or maybe there could be a way for a @load StatsFuns to call using StatsFuns and define the overloads. Even if it had to be called manually, that could be ok and might be a nice middle ground.

I guess I'll wait for @cjdoris to weigh in here, since it's his package and all :)

EDIT: Oops, I was typing and missed his comment ☝️

@cjdoris
Copy link
Owner

cjdoris commented May 8, 2022

Btw uexp is not API, you do exp(ULogarithmic, x).

@cjdoris
Copy link
Owner

cjdoris commented May 8, 2022

I agree that the ttfx issue can be overstated sometimes, but in this case the overloads aren't doing much. It's not hard for someone to define

pdf2(d, x) = exp(ULogarithmic, logpdf(d, x))

if they need it.

@oschulz
Copy link
Author

oschulz commented May 8, 2022

Btw uexp is not API, you do exp(ULogarithmic, x)

Speaking as a future user of LogarithmicNumbers, an official shortcut for exp(ULogarithmic, x) would be neat to have. How about lazyexp(x) = exp(Logarithmic, x) and lazyuexp(x) = exp(ULogarithmic, x) or so?

@oschulz
Copy link
Author

oschulz commented May 8, 2022

but I'm generally opposed to making real design decisions for the benefit of people who think using JIT compilation for a short-running script is a good idea. That use case can benefit from static compiler work, otherwise it's a terrible cost model.

I fully agree, short-running scripts are certainly not a target use case for Julia. But I think it's still worth trying to keep package load time down as much as possible in general.

For once, impressions matter, and in my experience package load times do immediately become a topic when advocating for (resp. introducing people tor Julia), no matter how often you tell people that they won't really start a new session that often.

More importantly though, we currently have a bit of a plague of @requires in the ecosystem, driven by package load times, packages depending on each other in technically "inverse" order and lack of common interface package in some areas. And long package load cause even more @requires, it's a vicious cycle. :-) And @require is never an idea solution. In my experience people are hesitant to take on packages with @requires on as low-level dependencies. And @require causes fragility because it's not compatible with semantic versioning (the @requires in LogarithmicNumbers are implemented very carefully, with feature checks, but that's not always the case).

@cjdoris
Copy link
Owner

cjdoris commented May 8, 2022

@oschulz Couldn't agree more, especially the semver part.

I'm happy to add a shorthand, but the perfect name isn't jumping out.

  • logarithmic_exp(x) is not much shorter than exp(ULogarithmic, x)
  • logexp(x) looks like it does log(exp(x))!
  • lazyexp(x) well I guess it is a lazy exp if you think about it, though laziness is not the primary feature, and the name doesn't obviously link it to this package.

@oschulz
Copy link
Author

oschulz commented May 8, 2022

I'm happy to add a shorthand, but the perfect name isn't jumping out.

I agree logarithmic_exp(x) and logexp(x) could mislead the user.

I think lazyexp(x) is semantically very sound, and a quick grep shows no other package (in my "packages" directory) using it. I'm admittedly biased, since I came up with the name, but maybe it's Ok if the name doesn't resemble the package name?

@gdalle
Copy link
Contributor

gdalle commented May 8, 2022

I like lazyexp the most, but if you're not convinced maybe we could find something along the line of safeexp (because it avoids overflow)?

@oschulz
Copy link
Author

oschulz commented May 8, 2022

I like lazyexp the most, but if you're not convinced

The "lazyness" may actually be useful part of it, in addition to preserving numerical precision and not exceeding float dynamical range. After all, in many cases there will be a log at the end of the calculation chain, so one saves and exp and a log, and those aren't the cheapest operations. Admittedly that will only have a significant beneficial performance impact of there aren't a lot of other expensive calculations between the lazyexp and the log, but still, it would justify the name. :-)

@oschulz
Copy link
Author

oschulz commented May 9, 2022

Here's a neat use case for LogarithmicNumbers: Nested sampling produces an integral estimate - unfortunately the uncertainty on that estimate is a Gaussian only in log-space. But LogarithmicNumbers allows us to do write (using lazyuexp here)

julia> using LogarithmicNumbers, Measurements
julia> lazyuexp(x) = exp(ULogarithmic, x)
julia> log_result = 4.2
julia> log_result_uncertainty = 0.1
julia> lazyuexp(log_result ± log_result_uncertainty)
exp(4.2 ± 0.1)

If lazyuexp (or different name) becomes part of the LogarithmicNumbers API, it might be nice to change the current Base.show specialization to

function Base.show(io::IO, x::ULogarithmic)
    print(io, "lazyuexp("); show(io, x.log); print(io, ")")
end

This would look nice, I think:

julia> lazyuexp(log_result ± log_result_uncertainty)
lazyuexp(4.2 ± 0.1)

@cscherrer
Copy link

Very nice, @oschulz !

FWIW, here's my use of this in MeasureBase.jl:

@inline function density_def(s::SuperpositionMeasure{Tuple{A,B}}, x) where {A,B}
    (μ, ν) = s.components

    insupport(μ, x) || return exp(ULogarithmic, logdensity_def(ν, x))
    insupport(ν, x) || return exp(ULogarithmic, logdensity_def(μ, x))
  
    α = basemeasure(μ)
    β = basemeasure(ν)
    dμ_dα = exp(ULogarithmic, logdensity_def(μ, x))
    dν_dβ = exp(ULogarithmic, logdensity_def(ν, x))
    dα_dβ = exp(ULogarithmic, logdensity_rel(α, β, x))
    dβ_dα = inv(dα_dβ)
    return dμ_dα / oneplus(dβ_dα) + dν_dβ / oneplus(dα_dβ)
end

We generally work in terms of log-densities instead of densities, in order to avoid underflow. But for superpositions, the density is comparatively simple, but log-density is very awkward to express. LogarithmicNumbers gives us the best of both!

MeasureBase is intended as a relatively low-level dependency, so avoiding @requries will be helpful, as you point out in the OP.

@cjdoris
Copy link
Owner

cjdoris commented May 9, 2022

Alright, LogarithmicNumbers is now dependency free! v1.2.0 is making its way through the package registration machine as we speak so will be available shortly.

@cjdoris
Copy link
Owner

cjdoris commented May 9, 2022

I think I'm going to not add lazyexp. You can always define it yourself if you feel it's convenient, but it feels like unnecessary sugar to be in this package (just like the overloads I just removed were!)

@cjdoris
Copy link
Owner

cjdoris commented May 9, 2022

Really enjoying the examples @oschulz and @cscherrer!

@cjdoris cjdoris closed this as completed May 9, 2022
@oschulz
Copy link
Author

oschulz commented May 9, 2022

Alright, LogarithmicNumbers is now dependency free!

Thanks a lot @cjdoris !

I think I'm going to not add lazyexp. [...] You can always define it yourself [...]

One advantage of having it in the package (maybe using another name) be that we could have a show with output that users can paste back in to recreate the object, instead of "exp(...)" which might (when pasted in) exceed float dynamic range.

@oschulz
Copy link
Author

oschulz commented May 9, 2022

And now for the reward ... :-)

Before:

julia> using InverseFunctions # Load a super-lightweight package to get some initial Pkg costs out of the way

julia> @time_imports using SpecialFunctions, StatsFuns, Distributions, LogarithmicNumbers
    [...]
    226.6 ms  SpecialFunctions
    [...]
     92.7 ms  StatsFuns
    [...]
    470.4 ms  Distributions
    [...]
    379.7 ms  LogarithmicNumbers

After:

julia> using InverseFunctions # Load a super-lightweight package to get some initial Pkg costs out of the way

julia> @time_imports using SpecialFunctions, StatsFuns, Distributions, LogarithmicNumbers
    204.0 ms  SpecialFunctions
    [...]
     79.7 ms  StatsFuns
    [...]
    483.4 ms  Distributions
    [...]
      9.4 ms  LogarithmicNumbers

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

5 participants