MultiDistribution

[benjamin-lieser]

• Added a CHANGELOG.md entry

Summary

Some code related to #16

[benjamin-lieser]

I put a &self receiver, like in Distribution @dhardy was there a specific reason why you proposed &mut self?

I am not sure if I want a different name, because it clashed with the one in Distribution and I would like structs to implement both if applicable. Making these things unambiguous feels often too cumbersome in Rust. And it can break existing code when doing use rand_distr::* or similar.

[dhardy]

I put a &self receiver, like in Distribution @dhardy was there a specific reason why you proposed &mut self?

Sorry, it should be &self.

I am not sure if I want a different name, because it clashed with the one in Distribution and I would like structs to implement both if applicable. Making these things unambiguous feels often too cumbersome in Rust. And it can break existing code when doing use rand_distr::* or similar.

I'm not really sure of the answer to that. Any of these can work (from the POV of a dependency):

• use rand_distr::MultiDistribution; — Do we want the multi module to be pub? Likely yes, in which case supporting this path is redundant.
• use rand_distr::multi::Distribution; — Usable but potential for confusion and can make usage of both Distribution traits annoying
• use rand_distr::multi::MultiDistribution; — Redundant naming, but avoids the above issues so probably the best choice

[benjamin-lieser]

I was talking about the naming of sample because its the same as in distribution and makes the method ambiguous when both traits are in scope.

I guess I also prefer the last of the 3 options for the name of the trait.

[benjamin-lieser]

Also, should we do this in a v0.6 branch? I guess there will be breaking changes with respect to the Dirichlet at the end and possible some v0.5.* releases.

[dhardy]

I was talking about the naming of sample because its the same as in distribution and makes the method ambiguous when both traits are in scope.

This shouldn't be an issue unless a type implements both Distribution and MultiDistribution — but I doubt we'd want that.

Except possibly a symmetric distribution like Dirichlet which could be sampled in one or multiple dimensions? Or if we wanted to transparently support uncorrelated multi-dimensional sampling of 1D distributions like Normal? No, both these are likely bad ideas.

[benjamin-lieser]

I was talking about the naming of sample because its the same as in distribution and makes the method ambiguous when both traits are in scope.

This shouldn't be an issue unless a type implements both Distribution and MultiDistribution — but I doubt we'd want that.

Except possibly a symmetric distribution like Dirichlet which could be sampled in one or multiple dimensions? Or if we wanted to transparently support uncorrelated multi-dimensional sampling of 1D distributions like Normal? No, both these are likely bad ideas.

I was thinking about both having MultiDistribution to sample into a buffer and still Distribution which returns a Vec. If someone would anyway allocate for each sample or does not care about allocations, the latter is more convenient.
But I could understand if this would lead to more confusion than it benefits people.

[dhardy]

The point of using const generics was to avoid needing to allocate. If allocation is necessary anyway, having both sample_to_buf and sample(...) -> Vec<_> doesn't add much. Moreover, if we are going to support both styles of method, it should be done under the same trait in my opinion — we can implement sample automatically, provided we know the expected sample length.

Which leads to another point: we may want a sample_len or just len method.

So:

pub trait MultiDistribution<T> {
    fn sample_len(&self) -> usize;
    fn sample_to_buf(&self, buf: &mut [T]);
    fn sample(&self) -> Vec<T> where T: Default {
        let mut buf = Vec::new();
        buf.resize_with(self.sample_len(), T::default());
        self.sample_to_buf(&mut buf);
    }
}

That requires T: Default to support sample, which I think is reasonable.

[benjamin-lieser]

We should decide if we want to keep the possibility to have multidim sampling without allocations or not. If we do not need it, we can ditch the const generics and have less code and then your proposed Trait would make a lot of sense.

I am still leaning toward keeping it, because I had a usecase where Multinomial samples where extremely time critical and it helps to save the allocations, especially in multithread where there is synchronization with malloc. But this might also be a niche usecase. (I would only sample once per Multinomial)

Edit: Your Trait is also still implementable for a const generic version and you can avoid all allocations. So I would go with this approach.

[dhardy]

I thought we did decide to drop the const-generics approach for rand_distr?

Your use-case sounds fairly specific. Maybe there are further optimisations available when sampling only once.

[MortenLohne]

Would it be possible to have a non-const generic Dirichlet that can still be used without allocating? Without discussing Distribution traits for now, imagine the following API:

impl<F: Float> Dirichlet<F> {
    pub fn new(alpha: Vec<F>) -> Result<Dirichlet<F>, Error>;
    pub fn sample<R: Rng>(&self, rng: &mut R, output: &mut Vec<F>); // Users can re-use this vector to avoid allocations
    pub fn into_vec(self) -> Vec<F>; // Recovers the memory passed in with `new()`
}

This still requires the alloc feature, but so does the current (const generic) implementation, and afaict no one has presented such a use case yet.

For sample(), we could also generalize the output type to iter::Extend, if we want to allow collecting into other data structures.

[benjamin-lieser]

Would it be possible to have a non-const generic Dirichlet that can still be used without allocating? Without discussing Distribution traits for now, imagine the following API:

impl<F: Float> Dirichlet<F> {
    pub fn new(alpha: Vec<F>) -> Result<Dirichlet<F>, Error>;
    pub fn sample<R: Rng>(&self, rng: &mut R, output: &mut Vec<F>); // Users can re-use this vector to avoid allocations
    pub fn into_vec(self) -> Vec<F>; // Recovers the memory passed in with `new()`
}

This still requires the alloc feature, but so does the current (const generic) implementation, and afaict no one has presented such a use case yet.

For sample(), we could also generalize the output type to iter::Extend, if we want to allow collecting into other data structures.

In the case of Dirichlet it needs an array of other distributions (Beta or Gamma), so this would not work. I would also say its a bit to complex of an API.

[dhardy]

Some comments on comment style below.

More significantly, the sample method now looks identical to Distribution::sample, hence your idea to use that trait likely makes more sense: we can automatically impl Distribution<Vec<T>> where T: Default.

This would also allow an explicit impl of Distribution<[T; N]> where appropriate (e.g. your mentioned const-generic Multinomial).

But, at this point, do we still want the MultiDistribution trait at all?

[dhardy]

We generally wrap comments at 80 chars width (sometimes up to 100 if the line already has a large indent).

The wording could be a little better, e.g.

The MultiDistribution trait allows sampling a multi-dimensional distribution to a pre-allocated buffer or to a new [Vec].

[dhardy]

Items have a short one-line description, with additional details in new paragraphs.

[benjamin-lieser]

My current favorite would be something like this:

pub trait MultiDistribution<T> {
    /// Returns the length of one sample (dimension of the distribution)
    fn sample_len(&self) -> usize;
    /// Samples from the distribution and writes the result to `buf`
    fn sample_to_buf<R: Rng + ?Sized>(&self, rng: &mut R, buf: &mut [T]);
}

impl<T, D> Distribution<Vec<T>> for D
where
    D: MultiDistribution<T>,
    T: Clone + Default,
{
    fn sample<R: Rng + ?Sized>(&self, rng: &mut R) -> Vec<T> {
        let len = self.sample_len();
        let mut buf = vec![Default::default(); len];
        self.sample_to_buf(rng, &mut buf);
        buf
    }
}

The distributions would implement MultiDistribution and we get the Distribution<Vec<T>> implementation for free.

Unfortunately this does not work because of orphan rules. It also does not feel right to define the MultiDistribution in rand, so I am not sure if this can work.

We could just implement Distribution manually (we could then also distinguish between Vec<T> and [T;3] for example).

[dhardy]

I think we can go with this trait design.

[dhardy]

Since the length of output affects behaviour, I would expect an assert on it.

[dhardy]

This one too.

[dhardy]

This method should be called sample_to_slice in my opinion. Or perhaps just sample_to.