Support for Dirichlet distribution #485

rohitjoshi · 2018-05-30T03:01:50Z

Added support for the Dirichlet distribution.

Fixes #400

dhardy

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

dhardy · 2018-05-30T09:51:07Z

src/distributions/dirichlet.rs

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


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.

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

dhardy · 2018-05-30T09:53:20Z

src/distributions/dirichlet.rs

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


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

dhardy · 2018-05-30T09:55:52Z

src/distributions/dirichlet.rs

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


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.

dhardy · 2018-05-30T09:57:50Z

src/distributions/dirichlet.rs

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


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.

dhardy · 2018-05-30T09:59:35Z

src/distributions/dirichlet.rs

+    #[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");


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

vks · 2018-05-30T11:00:10Z

src/distributions/mod.rs

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


The [`Dirichlet`] link is missing.

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

rohitjoshi · 2018-05-31T01:20:02Z

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

dhardy · 2018-05-31T07:53:05Z

src/distributions/dirichlet.rs

-        Dirichlet {
-            alpha: alpha.to_vec(),
-        }
+        Dirichlet { alpha: a.into() }


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

dhardy · 2018-05-31T07:56:12Z

src/distributions/mod.rs

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


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

dhardy · 2018-05-31T07:57:10Z

src/distributions/dirichlet.rs

+            assert!(a[i] > 0.0);
+        }
+
+        Dirichlet { alpha: a.into() }


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 · 2018-06-01T20:30:52Z

@dhardy addressed review comment.

dhardy · 2018-06-02T16:05:16Z

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

vks · 2018-06-05T10:57:20Z

src/distributions/dirichlet.rs

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


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

vks · 2018-06-05T10:58:42Z

src/distributions/dirichlet.rs

+/// 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


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

vks · 2018-06-05T11:02:24Z

src/distributions/dirichlet.rs

+    }
+}
+
+impl Distribution<Vec<f64>> for Dirichlet {


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]).

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).

dhardy · 2018-06-05T18:43:40Z

src/distributions/dirichlet.rs

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


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).

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

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

pitdicker · 2018-06-10T06:28:26Z

I think it is just time to merge this PR?

dhardy · 2018-06-10T07:31:54Z

src/distributions/mod.rs

@@ -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


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

dhardy · 2018-06-10T07:33:26Z

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

Rohit Joshi added 3 commits May 29, 2018 22:59

adding support for dirichlet distribution

546caa8

modifying to support stable version

14462ca

added feature std

c530db7

dhardy reviewed May 30, 2018

View reviewed changes

vks reviewed May 30, 2018

View reviewed changes

Rohit Joshi added 5 commits May 30, 2018 20:24

adding a link

57fb716

review comments

aad2f45

Merge branch 'master' of https://github.com/rust-lang-nursery/rand

4d1cc6c

removing extra empty line

ad13516

added std feature

d7a5ced

removed stray }

d728989

dhardy reviewed May 31, 2018

View reviewed changes

addressing review comments

732209a

doc markup update

2f706a6

dhardy added T-distributions Topic: distributions P-medium Priority: Medium D-review Do: needs review labels Jun 2, 2018

Merge branch 'master' of https://github.com/rust-lang-nursery/rand

fde9567

vks reviewed Jun 5, 2018

View reviewed changes

vks mentioned this pull request Jun 5, 2018

Better support for multivariate distributions #496

Open

dhardy reviewed Jun 5, 2018

View reviewed changes

dhardy added D-review Do: needs review D-changes Do: changes requested and removed D-review Do: needs review labels Jun 8, 2018

dhardy removed the D-review Do: needs review label Jun 8, 2018

dhardy reviewed Jun 10, 2018

View reviewed changes

dhardy merged commit 187b7d1 into rust-random:master Jun 12, 2018

dhardy added a commit that referenced this pull request Jun 12, 2018

Fix documentation issues from #485

8fdd710

dhardy mentioned this pull request Jun 20, 2018

Tracker: 0.6 release #520

Closed

28 tasks

sync-by-unito bot mentioned this pull request Dec 15, 2020

Bump rand_core from 0.5.1 to 0.6.0 AleoHQ/leo#503

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for Dirichlet distribution #485

Support for Dirichlet distribution #485

rohitjoshi commented May 30, 2018

dhardy left a comment

dhardy May 30, 2018

vks May 30, 2018

dhardy May 30, 2018

dhardy May 30, 2018

dhardy May 30, 2018

dhardy May 30, 2018

vks May 30, 2018

dhardy May 31, 2018

rohitjoshi commented May 31, 2018

dhardy May 31, 2018

dhardy May 31, 2018

dhardy May 31, 2018

rohitjoshi commented Jun 1, 2018

dhardy commented Jun 2, 2018

vks Jun 5, 2018

vks Jun 5, 2018

vks Jun 5, 2018

dhardy Jun 5, 2018

dhardy Jun 5, 2018 •

edited

pitdicker Jun 7, 2018

dhardy Jun 7, 2018 •

edited

pitdicker commented Jun 10, 2018

dhardy Jun 10, 2018

dhardy commented Jun 10, 2018

Support for Dirichlet distribution #485

Support for Dirichlet distribution #485

Conversation

rohitjoshi commented May 30, 2018

dhardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohitjoshi commented May 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohitjoshi commented Jun 1, 2018

dhardy commented Jun 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhardy Jun 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhardy Jun 7, 2018 • edited

Choose a reason for hiding this comment

pitdicker commented Jun 10, 2018

Choose a reason for hiding this comment

dhardy commented Jun 10, 2018

dhardy Jun 5, 2018 •

edited

dhardy Jun 7, 2018 •

edited