PDF operators for each distribution #14579

david-seiler · 2019-04-01T09:53:34Z

Description

This PR adds operators for computing the densities of samples drawn from any of the various distributions defined in operator/random, as well as their gradients. There are lots of changes to test_random.py to test each PDF alongside its distribution; aside from that, the patch should be entirely stand-alone. See pdf_op.cc for more-detailed description strings.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage:
Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
Code is well-documented:
For user-facing API changes, API doc string has been updated.
For new C++ functions in header files, their functionalities and arguments are documented.
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

asmushetzel · 2019-04-01T13:46:37Z

src/operator/random/pdf_op.cc

+produces output of dimension *n+1* such that each *n*-dimensional index *i*
+in the output array holds the PDFs of the samples at index *i* in *sample*,
+parameterized by the values of *low* and *high* at index *i*.
+


A special case is that sample is also an n-dimensional tensor which means that we have exactly one sample per distribution. In that case, the output will also be an n-dimensional tensor. I think the code does (and should) support this and that this is an important use case. So should be documented here.

Called that out explicitly.

asmushetzel · 2019-04-01T13:47:20Z

src/operator/random/pdf_op.cc

+
+Examples::
+
+    random_pdf_uniform(sample=[[1,2,3,4]], low=[0], high=[10]) = [0.1 0.1 0.1 0.1]


Commas missing in the output array.

asmushetzel · 2019-04-01T13:52:06Z

src/operator/random/pdf_op.cc

+
+  random_pdf_exponential(sample=[[1, 2, 3]], lam=[1]) =
+      [[0.36787945 0.13533528 0.04978707]]
+


We should have consistent types of examples for all distributions.

single distribution, multiple samples

multiple distributions, multiple samples

multiple distributions, single sample per distribution

Added second examples for the exponential and Dirichlet.

asmushetzel · 2019-04-01T14:03:04Z

src/operator/random/pdf_op.h

+  bool is_log;
+  DMLC_DECLARE_PARAMETER(PdfParam) {
+    DMLC_DECLARE_FIELD(is_log).set_default(false)
+    .describe("Whether to compute the log PDF or not.");


May be that can be phrased a bit better.

It's now "If set, compute the density of the log-probability instead of the probability."

asmushetzel · 2019-04-01T14:17:12Z

src/operator/random/pdf_op.h

+  template<typename DType, typename IType1, typename IType2>
+  MSHADOW_XINLINE static void Map(int start, int length, int sample_size, int index,
+                                  DType *out, IType1 *sample, IType2 *lower, IType2 *upper) {
+    const DType l(lower[index]), h(upper[index]);


Providing "index" to this and all similar functions below is redundant. According to the processing in LaunchExWrapper it always hold that index == start/sample_size. Still we may want to keep "index" as an explicit parameter for clarity. So o.k. with either choice.

asmushetzel · 2019-04-01T14:21:12Z

Should be noted that this PR allows to compute log-pdf and pdf. And that it also adds pdf of Dirichlet distribution which does not yet exist in MXNet.
For consistency, we should then add also a sampler for Dirichlet-distributions after this PR gets merged.

asmushetzel · 2019-04-01T14:28:38Z

This addresses several requests from issue #12932

anirudhacharya · 2019-04-01T18:39:57Z

src/operator/random/pdf_op.cc

+)code");
+}
+
+inline std::string dirichlet_desc() {


I don't think we have a function to draw samples from a dirichlet distribution? this only computes the pdf of the distribution?

yep. Sampler does not exist yet. As by my previous comments, we should add it. We may have the bandwidth to do so in our team (and actually will need it). But we should not make existence of a sampler be a conditional to this PR as having the PDF is beneficial by itself (and we are using it already in a project).
My hope is that by adding serious functionality to the random-namespace in MXNet, we get more people interested (for example supplying also CDF etc.)

Updated the commit message to clarify that, unlike the other distributions, we don't have a Dirichlet sampler yet.

marcoabreu · 2019-04-01T21:22:14Z

Is this PR pointing against the 1.3 branch on purpose?

piyushghai · 2019-04-02T17:57:18Z

@david-seiler Seems like you want to add a particular feature to a previous version of MXNet.
The correct process to do this would be :

Raise a PR against the master branch.
After that PR is merged into master, cherry-pick the same commit in another PR raised against v1.3.x (Or a different version) branch.

@mxnet-label-bot Add [pr-awaiting-review, operator]

david-seiler · 2019-04-03T07:42:31Z

@david-seiler Seems like you want to add a particular feature to a previous version of MXNet.

I started with 1.3.x because it's what we're using internally (for now); I'll close this in favor of a new PR against master once I've updated to build against master and addressed the rest of the review comments.

…r (plus also the PDF of the Dirichlet). Supports probabilities and log-probabilities, as well as gradients.

david-seiler · 2019-04-03T13:45:28Z

This PR now correctly targets master -- still from a branch named v1.3.x on my side, since I couldn't see how to reset that, but that branch now follows apache:master with this one commit on top. Sorry for the confusion, but it seems better on net than throwing away the existing conversation.

david-seiler · 2019-04-04T15:29:16Z

The tests in Jenkins have been stuck ever single I switched this PR to master, and I haven't found a way to unstick them, so please join me in a new PR of the same code: #14617

All the comments from this PR should be addressed there.

david-seiler requested a review from anirudh2290 as a code owner April 1, 2019 09:53

david-seiler force-pushed the v1.3.x branch 2 times, most recently from 9ee1b6e to 5733e78 Compare April 1, 2019 13:50

asmushetzel reviewed Apr 1, 2019

View reviewed changes

asmushetzel mentioned this pull request Apr 1, 2019

Probability Distributions Support #12932

Open

anirudhacharya reviewed Apr 1, 2019

View reviewed changes

david-seiler force-pushed the v1.3.x branch 2 times, most recently from a2e601c to 971ae83 Compare April 2, 2019 15:37

marcoabreu added Operator pr-awaiting-review PR is waiting for code review labels Apr 2, 2019

PDF operators for each distribution for which we have a random sample…

a8a23f6

…r (plus also the PDF of the Dirichlet). Supports probabilities and log-probabilities, as well as gradients.

david-seiler force-pushed the v1.3.x branch from 971ae83 to f4f3e47 Compare April 3, 2019 13:41

david-seiler changed the base branch from v1.3.x to master April 3, 2019 13:42

david-seiler force-pushed the v1.3.x branch from f4f3e47 to a8a23f6 Compare April 3, 2019 13:42

david-seiler requested review from gigasquid, marcoabreu, nswamy, sergeykolychev, szha and yzhliu as code owners April 3, 2019 13:42

david-seiler mentioned this pull request Apr 4, 2019

PDF operators for the random samplers, and also the Dirichlet #14617

Merged

4 tasks

david-seiler closed this Apr 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PDF operators for each distribution #14579

PDF operators for each distribution #14579

david-seiler commented Apr 1, 2019

asmushetzel Apr 1, 2019

david-seiler Apr 3, 2019

asmushetzel Apr 1, 2019

asmushetzel Apr 1, 2019

david-seiler Apr 3, 2019

asmushetzel Apr 1, 2019

david-seiler Apr 3, 2019

asmushetzel Apr 1, 2019

david-seiler Apr 3, 2019

asmushetzel commented Apr 1, 2019

asmushetzel commented Apr 1, 2019

anirudhacharya Apr 1, 2019

asmushetzel Apr 1, 2019

david-seiler Apr 3, 2019

marcoabreu commented Apr 1, 2019

piyushghai commented Apr 2, 2019

david-seiler commented Apr 3, 2019

david-seiler commented Apr 3, 2019

david-seiler commented Apr 4, 2019


		Examples::

		random_pdf_uniform(sample=[[1,2,3,4]], low=[0], high=[10]) = [0.1 0.1 0.1 0.1]


		random_pdf_exponential(sample=[[1, 2, 3]], lam=[1]) =
		[[0.36787945 0.13533528 0.04978707]]

PDF operators for each distribution #14579

PDF operators for each distribution #14579

Conversation

david-seiler commented Apr 1, 2019

Description

Checklist

Essentials

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

asmushetzel commented Apr 1, 2019

asmushetzel commented Apr 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoabreu commented Apr 1, 2019

piyushghai commented Apr 2, 2019

david-seiler commented Apr 3, 2019

david-seiler commented Apr 3, 2019

david-seiler commented Apr 4, 2019