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

Support for Dirichlet distribution #485

Merged
merged 12 commits into from
Jun 12, 2018
Merged

Conversation

rohitjoshi
Copy link

Added support for the Dirichlet distribution.

Fixes #400

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks equivalent to the GSL implementation, except that the GSL has special handling for small values. See GSL source.


/// The dirichelet distribution `Dirichlet(alpha)`.
///
/// The Dirichlet distribution } is a family of continuous multivariate probability distributions parameterized by
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stray }

Copying a fancy description from Wikipedia doesn't really explain much, especially since the links are missing. Not that I have a better idea.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the Mathematica explanation a bit more than Wikipedia's.

for i in 0..alpha.len() {
assert!(
alpha[i] > 0.0,
"Dirichlet::new called with `alpha` <= 0.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think you need the quotes. Maybe just write alpha[i] <= 0, and alpha.len() < 2 above.

/// Construct a new `Dirichlet` with the given alpha parameter
/// `alpha`. Panics if `alpha.len() < 2`.
#[inline]
pub fn new(alpha: &[f64]) -> Dirichlet {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest you accept any vec-like thing here: new<V: Into<Vec<f64>>(alpha: V) and then let alpha = alpha.into();. Drop the to_vec() later.


/// Construct a new `Dirichlet` with the given shape parameter and size
/// `alpha`. Panics if `alpha <= 0.0`.
/// `size` . Panic if `size < 2`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't render well. If you want a list, leave a blank line, the prefix each item with - (it's Markdown). Otherwise just rewrite as two sentences.

#[inline]
pub fn new_with_param(alpha: f64, size: usize) -> Dirichlet {
assert!(alpha > 0.0, "Dirichlet::new called with `alpha` <= 0.0");
assert!(size > 1, "Dirichlet::new called with `size` <= 1");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is new_with_param not new. Again you can drop the extra quotes on parameter names.

@@ -95,7 +95,7 @@
//! - [`ChiSquared`] distribution
//! - [`StudentT`] distribution
//! - [`FisherF`] distribution
//!
//! - Dirichlet distribution
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The [`Dirichlet`] link is missing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been added now, but Dirichlet seems to be mentioned twice in a row

@rohitjoshi
Copy link
Author

@dhardy @vks thanks for your feedback. I have tried to address your review comments.

Dirichlet {
alpha: alpha.to_vec(),
}
Dirichlet { alpha: a.into() }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need into() again here — it's already a Vec

@@ -95,7 +95,7 @@
//! - [`ChiSquared`] distribution
//! - [`StudentT`] distribution
//! - [`FisherF`] distribution
//!
//! - Dirichlet distribution
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been added now, but Dirichlet seems to be mentioned twice in a row

assert!(a[i] > 0.0);
}

Dirichlet { alpha: a.into() }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bit. Not sure why my last comment went somewhere else. a is already your target type, so you don't need .into() again.

@rohitjoshi
Copy link
Author

@dhardy addressed review comment.

@dhardy dhardy added T-distributions D-review Do: needs review labels Jun 2, 2018
@dhardy
Copy link
Member

dhardy commented Jun 2, 2018

Thanks @rohitjoshi! Looks good enough to me. Does anyone else want to comment before merging?


/// The dirichelet distribution `Dirichlet(alpha)`.
///
/// The Dirichlet distribution is a family of continuous multivariate probability distributions parameterized by
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is a bit long. I think we usually wrap comments at 80 characters.

/// The dirichelet distribution `Dirichlet(alpha)`.
///
/// The Dirichlet distribution is a family of continuous multivariate probability distributions parameterized by
/// a vector alpha of positive reals. https://en.wikipedia.org/wiki/Dirichlet_distribution
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naked link will look weird in the docs. I think you can remove, there is not precedence in Rand for linking to Wikipedia.

}
}

impl Distribution<Vec<f64>> for Dirichlet {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure our current distribution trait is well suited for multivariate distributions. It would be nice to sample without allocating, but this requires different method. Something like fn sample_multi(&self, &mut Rng, &mut [f64]).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why you opened #496. I agree. On the other hand, I'm not too fussed about having to make breaking changes to this distribution later (it's still better for users than not having it, and we're not close to 1.0).

#[derive(Clone, Debug)]
pub struct Dirichlet {
/// Concentration parameters (alpha)
alpha: Vec<f64>,
Copy link
Member

@dhardy dhardy Jun 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it, we could probably use alpha: [f64] here. It makes the type "unsized" (i.e. users have to write Box<Dirichlet>) but is more flexible (potentially more optimal).

On the other hand it may not be worth it since it makes the type less ergonomic to use for what is probably not a lot of gain.

Another option would be Dirichlet<N: usize> { alpha: [f64; N] } — except I don't think Rust supports that yet (though it would also allow sample(..) -> [f64; N], thus side-stepping @vks's concerns).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do our distributions even work when you write Box<Dirichlet>?

Copy link
Member

@dhardy dhardy Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rng::sample won't of course but Distribution::sample will. Either way it's not really a great choice (less convenient for users).

@dhardy dhardy added D-review Do: needs review D-changes Do: changes requested and removed D-review Do: needs review labels Jun 8, 2018
@dhardy dhardy removed the D-review Do: needs review label Jun 8, 2018
@pitdicker
Copy link
Contributor

I think it is just time to merge this PR?

@@ -81,7 +81,6 @@
//! - Related to real-valued quantities that grow linearly
//! (e.g. errors, offsets):
//! - [`Normal`] distribution, and [`StandardNormal`] as a primitive
//! - [`Cauchy`] distribution
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rohitjoshi did you intend to move the reference to Cauchy in this documentation? Because you haven't added it back.

@dhardy
Copy link
Member

dhardy commented Jun 10, 2018

Maybe @pitdicker. It still needs a couple of doc tweaks, but of course any of us can do that too.

@dhardy dhardy merged commit 187b7d1 into rust-random:master Jun 12, 2018
dhardy added a commit that referenced this pull request Jun 12, 2018
@dhardy dhardy mentioned this pull request Jun 20, 2018
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D-changes Do: changes requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants