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

Better support for multivariate distributions #496

Closed
vks opened this issue Jun 5, 2018 · 4 comments
Closed

Better support for multivariate distributions #496

vks opened this issue Jun 5, 2018 · 4 comments
Labels
E-question Participation: opinions wanted P-low Priority: Low

Comments

@vks
Copy link
Collaborator

vks commented Jun 5, 2018

The Distribution trait does not work well for multivariate distributions, because impl Distribution<Vec<f64>> for Multivariate requires an allocation for every sample (see #485 for an example). Maybe we should add another trait:

pub trait MultivariateDistribution<T> {                                                         
    fn sample_multi<R: Rng + ?Sized>(&self, rng: &mut R, buffer: &mut [T]);
}

With SIMD in mind, this might even make sense for univariate distribution. So we might want to add sample_multi to the Distribution trait.

@dhardy
Copy link
Member

dhardy commented Jun 6, 2018

This is a good point. My preference is to postpone this until we resolve #290 or have more multi-variate distributions.

As mentioned in #485 there might be another solution, if fixing the number of variables at compile-time is sufficient: using generic consts (rust-lang/rust#44580).

@dhardy dhardy added E-question Participation: opinions wanted T-distributions P-low Priority: Low labels Jun 6, 2018
@vks
Copy link
Collaborator Author

vks commented Jun 6, 2018

Nice, I did not know const generics were so close!

rust-lang/rust#51192 (comment)

Given that const generics on nightly might be only a month away, my vote would be to wait...

@dhardy
Copy link
Member

dhardy commented May 21, 2024

We now use const generics: impl<F, const N: usize> Distribution<[F; N]> for Dirichlet<F, N> where ...

Caveat: we currently use Vec to construct some arrays in distribution constructors. Once core::array::try_from_fn is stable, we can use that instead.

Caveat: sometimes arrays of other lengths are required (DirichletFromBeta uses an internal array of length one less than the output length). This isn't really an issue, but currently uses an allocator (doing otherwise would require generic_const_exprs or an empty element).

Caveat: the above approach does not support run-time variable length N. We likely don't need to care about this.


Conclusion: we can improve our code once try_from_fn is stabilized, but probably not beyond that.

I tried simply copying the code behind try_from_fn as a utility fn; this is viable but requires rather more unsafe code than I'd like (besides which, we forbid unsafe from rand_distr).

@dhardy
Copy link
Member

dhardy commented May 21, 2024

Closing: #1006 tracks the remaining issues in Dirichlet, and I don't think there's anything else we want for Distribution.

@dhardy dhardy closed this as completed May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-question Participation: opinions wanted P-low Priority: Low
Projects
None yet
Development

No branches or pull requests

2 participants