Add Cumulative Distribution Function, Inverse CDF methods to Distributions by vishwakftw · Pull Request #122 · probtorch/pytorch

vishwakftw · 2018-02-03T04:57:34Z

Work in parallel with PR #121.

1. Cauchy 2. Exponential 3. Laplace (Only CDF) 4. Pareto

fritzo

Looks good! I only have minor comments about testing.

fritzo · 2018-02-04T04:35:23Z

+        set_rng_seed(0)  # see Note [Randomized statistical tests]
+        for pytorch_dist, scipy_dist in self.distribution_pairs:
+            samples = pytorch_dist.sample((5,))
+            try:


It's safer to enclose as little as needed in a try-except. Could you refactor to

try: cdf = pytorch_dist.cdf(samples) except NotImplementedError: continue self.assertEqual(cdf, scipy_dist.cdf(samples), message=pytorch_dist)

Ah, yes. I saw the discussion in TruncatedNormal. I will modify it accordingly.

fritzo · 2018-02-04T04:35:40Z

+        set_rng_seed(0)  # see Note [Randomized statistical tests]
+        for pytorch_dist, scipy_dist in self.distribution_pairs:
+            samples = Variable(torch.rand((5,) + pytorch_dist.batch_shape))
+            try:


ditto, enclose as little as possible in try-except

fritzo · 2018-02-04T04:36:31Z

        self._validate_log_prob_arg(value)
        return -torch.log(2 * self.scale) - torch.abs(value - self.loc) / self.scale

+    def cdf(self, value):


No .icdf()?

Laplace's .cdf is a piecewise function. I was doubtful about adding an inverse, and later realized that the inverse could be piecewise as well. Will update this too..

fritzo · 2018-02-04T04:44:20Z

                self.assertEqual(pytorch_dist.variance, scipy_dist.var(), allow_inf=True, message=pytorch_dist)
                self.assertEqual(pytorch_dist.stddev, scipy_dist.var() ** 0.5, message=pytorch_dist)

+    def test_cdf(self):


It would be nice to have an additional test that did not rely on scipy, e.g.

class TestDistributions(TestCase): def test_cdf_icdf(self): for Dist, params in EXAMPLES: for i, param in enumerate(params): dist = Dist(**param) samples = dist.sample(sample_shape=(20,)) try: cdf = dist.cdf(samples) actual = dist.icdf(cdf) except NotImplementedError: continue self.assertEqual(actual, samples, message='{} example {}/{}, icdf(cdf(x)) != x')

or you could get even fancier by using grad() like

x = dist.sample(sample_shape=(20,)) expected_pdf = dist.log_prob(x).exp() actual_pdf = grad(dist.cdf(x).sum(), [x])[0] self.assertEqual(actual_pdf, expected_pdf)

fritzo · 2018-02-04T07:35:50Z

TransformedDistribution would be cool to implement. Also Uniform, since a transformed uniform is basically a inverse-cdf sampler.

Minor: 1. Convert Pareto and Gumbel to TransformedDistribution 2. Add .cdf and .icdf for Uniform 3. Temporarily remove .cdf from Laplace

vishwakftw · 2018-02-04T18:46:32Z

Three tests fail:

~~Line 1770, Error: AssertionError: ValueError not raised by log_prob.~~
~~Line 980, Error: AssertionError: 2 != 1.~~
Line 1192, Error: Assertion Error: False is not true : TransformedDistribution example 1/3, d(cdf)/dx != pdf(x)

…ative of .cdf

fritzo

Looks great! Just one minor comment about eps and tiny, then it's ready to send upstream.

fritzo · 2018-02-05T16:11:01Z

-        z = (value - self.loc) / self.scale
-        return -(self.scale.log() + z + torch.exp(-z))
+        base_dist = Uniform(torch.zeros_like(self.loc), 1)
+        transforms = [ExpTransform().inv, AffineTransform(loc=0, scale=-1),


fritzo · 2018-02-05T16:13:06Z

-        self._validate_log_prob_arg(value)
-        z = (value - self.loc) / self.scale
-        return -(self.scale.log() + z + torch.exp(-z))
+        base_dist = Uniform(torch.zeros_like(self.loc), 1)


Maybe we should avoid infinity like

finfo = _finfo(self.loc) base_dist = Uniform(self.loc.new([finfo.tiny]).expand_as(self.loc), 1 - finfo.eps)

fritzo · 2018-02-05T16:17:29Z

+        Computes the inverse cumulative distribution function using transform(s) and computing
+        the score of the base distribution
+        """
+        self.base_dist._validate_log_prob_arg(value)


I believe the base_dist.icdf() should call _validate_log_prob_arg(value) internally on the following line. Do you think it's worth having the extra check here? I'd be happy either way.

fritzo · 2018-02-05T17:15:54Z

@vishwakftw Let me know if you want any help with the failing tests. I might have time today or tomorrow to help debug.

1. Fix the size issue with Gumbel as a transformed distribution 2. Add the scalar params test for Gumbel

vishwakftw · 2018-02-06T04:26:13Z

@fritzo I have fixed the shaping failures with the Gumbel distribution.

There is one issue however. Some how the TransformedDistribution test for test_cdf_log_prob fails, since the log_prob is a nan value for a sample.

vishwakftw · 2018-02-06T05:01:08Z

I also tried implementing Laplace as a TransformedDistribution. There is one missing piece however, have a look at the code below:

def __init__(self, loc, scale):
    self.loc, self.scale = broadcast_all(loc, scale)
    finfo = _finfo(self.loc)
    if isinstance(loc, Number) and isinstance(scale, Number):
        base_dist = Uniform(finfo.eps - 1, 1)
    else:
        base_dist = Uniform(self.loc.new([finfo.eps]).expand_as(self.loc) - 1, 1)
    transforms = [AbsTransform(), AffineTransform(loc=1, scale=-1), ExpTransform().inv,
                          AffineTransform(loc=self.loc, scale=self.scale)]
    super(Laplace, self).__init__(base_dist, transforms)

I believe the sampling requires a SignTransform, and I couldn't implement one using existing transforms. Something like this should work:

SignTransform = AbsTransform / identity_transform

…nsformed Distribution

fritzo

LGTM Feel free to send upstream.

vishwakftw · 2018-02-06T17:08:40Z

Great. I am sending this upstream now!!

alicanb and others added 2 commits February 3, 2018 09:34

add cdf and icdf to normal

e4e58e2

New CDF and ICDF implementations

d9263a6

1. Cauchy 2. Exponential 3. Laplace (Only CDF) 4. Pareto

vishwakftw added the WIP label Feb 3, 2018

vishwakftw self-assigned this Feb 3, 2018

fritzo reviewed Feb 4, 2018

View reviewed changes

Major: Add .cdf and .icdf methods for TransformedDistributions

ee55d13

Minor: 1. Convert Pareto and Gumbel to TransformedDistribution 2. Add .cdf and .icdf for Uniform 3. Temporarily remove .cdf from Laplace

vishwakftw added 3 commits February 5, 2018 11:44

Add SciPy / NumPy independent tests for cdf and icdf invertibility

f76114a

Addition of NumPy / SciPy independent tests for .log_prob using deriv…

d0bc72c

…ative of .cdf

Merge branch 'master' into cdf-icdf-methods

864d190

fritzo reviewed Feb 5, 2018

View reviewed changes

Bug fixes

a9d74ce

1. Fix the size issue with Gumbel as a transformed distribution 2. Add the scalar params test for Gumbel

vishwakftw and others added 2 commits February 6, 2018 21:15

Remove batch_shape calculation after making Pareto distribution a Tra…

24c2461

…nsformed Distribution

Fix out-of-range test parameters

2932ffe

fritzo approved these changes Feb 6, 2018

View reviewed changes

vishwakftw removed the WIP label Feb 6, 2018

vishwakftw closed this Feb 6, 2018

Conversation

vishwakftw commented Feb 3, 2018

Uh oh!

fritzo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vishwakftw Feb 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fritzo commented Feb 4, 2018

Uh oh!

vishwakftw commented Feb 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fritzo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fritzo commented Feb 5, 2018

Uh oh!

vishwakftw commented Feb 6, 2018

Uh oh!

vishwakftw commented Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fritzo left a comment

Choose a reason for hiding this comment

Uh oh!

vishwakftw commented Feb 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vishwakftw Feb 4, 2018 •

edited

Loading

vishwakftw commented Feb 4, 2018 •

edited

Loading

vishwakftw commented Feb 6, 2018 •

edited

Loading