JuliaStats / Distributions.jl

A Julia package for probability distributions and associated functions.
Other
1.08k stars 410 forks source link

Export pdf! and logpdf! #656

Open oschulz opened 6 years ago

oschulz commented 6 years ago

IMHO, pdf! and logpdf! come in very handy when repeatedly calculating multiple pdf values (re-use of output array, avoiding frequent memory allocation). I think they should be exported.

andreasnoack commented 6 years ago

We should probably rename these to broadcast! such that you'd write

out .= pdf.(D, v)

instead.

simonbyrne commented 6 years ago

I've actually deprecated these as part of #655.

oschulz commented 6 years ago

@andreasnoack > out .= pdf.(D, v)

That would be elegant - is that possible ('cause out would be a vector, and v a matrix)?

@simonbyrne > I've actually deprecated these as part of #655.

Is there a way to achieve the same functionality after the deprecation? I found pdf! and logpdf! to be very useful, to limit memory allocation. Will be a different story if/when Julia gets alloc-free arrays views, of course.

oschulz commented 6 years ago

@oschulz > That would be elegant - is that possible ('cause out would be a vector, and v a matrix)?

Sorry, I forgot to mention - I'm talking about multivariate distributions here. In the univariate case, it's simple, of course.

andreasnoack commented 6 years ago

Sorry, I forgot to mention - I'm talking about multivariate distributions here.

Ah. Makes sense now. The multivariate case is a different story.

We have actually been discussing to move the multivariate distributions to a separate because their interface is slightly different. One of the things I'd like to explore if we do that is to use StaticArrays much more for the multivariate distributions. The parameters could be stored much more efficiently as SVector and SMatrix but the variates could also be SVectors. Since a Vector{SVector{N,T}} has the same memory layout as a Matrix{T} we could still support BLAS operations and we could provide easy conversions to Matrix when necessary.

oschulz commented 6 years ago

The parameters could be stored much more efficiently as SVector and SMatrix but the variates could also be SVectors

That would certainly come in handy in some cases. But IMHO it's very important to keep support for standard arrays, too. I'm currently writing some MCMC code, for example, and the number of parameters can be large and can also depend on input data.

andreasnoack commented 6 years ago

My thinking is that it should be parametric on the array type such that both would be supported.

oschulz commented 6 years ago

My thinking is that it should be parametric on the array type

That would be great!

oschulz commented 6 years ago

So what about pdf! and logpdf! - I guess if they're about to be deprecated in the univariate case you probably don't want to export them for the multivariate case? But there should be some official way to generate multiple pdf values at once, because the internal implementation for that could be more efficient than just generating the values in sequence (plus, there's the memory-allocation aspect).

andreasnoack commented 6 years ago

Given the "plan" mentioned adove, I'd be reluctant to export pdf! and logpdf!. If we begin to use StaticArrays for multivariate distributions, you'd be able to use broadcast by supplying the evaluation points as Vector{SMatrix} instead of a Matrix. We could overload broadcast! for this case such that it could still be efficient.

simonbyrne commented 6 years ago

One option for multivariate distributions would be to overload mapslices, or create a separate function pdfslices or some such.

oschulz commented 6 years ago

I like the idea - but unfortunately, there doesn't seem to be a mapslices! (why not?). Otherwise, it would seem ideal. But if we have to use a separate function, wouldn't pdf! be a more natural name than pdfslices? After all, rand and rand! also allow for production of multiple outputs for multivariate distributions.

simonbyrne commented 6 years ago

Possibly, but it would be nice if it was consistent with other slicing functionality. I'm still thinking about the best way to do this.