Add random kwarg to DensityDist #2106 by Vaibhavdixit02 · Pull Request #2805 · pymc-devs/pymc

Vaibhavdixit02 · 2018-01-18T07:38:48Z

As per the discussion with @twiecki in #2106 I have implemented the random method for DensityDist, this is my first PR in PyMC3 so any feedbacks are very welcome. 😄

twiecki · 2018-01-18T10:28:05Z

pymc3/distributions/distribution.py

I would move this to before *args to not mess with the order.

twiecki · 2018-01-18T10:29:00Z

pymc3/distributions/distribution.py

I don't think this works, you need to call self.rand(*args, **kwargs) and add those as well to the method call signature.

Yeah that makes more sense. 👍

twiecki · 2018-01-18T10:29:17Z

That's a good start, definitely needs a test.

Vaibhavdixit02 · 2018-01-18T11:39:39Z

Which file would be appropriate for its test? I am confused between test_distributions.py and test_distributions_random.py

twiecki · 2018-01-18T12:07:53Z

I would add a test to https://github.com/pymc-devs/pymc3/blob/718dab9ded5bf99d2fcfcd4ec8e2cf9769bb216a/pymc3/tests/test_distributions_random.py where you create a DensityDist for the Normal distribution (so the random function is trivial) and test it like we do the Normal random method.

twiecki · 2018-01-18T12:08:15Z

pymc3/distributions/distribution.py

PEP8 requires spaces after arguments

twiecki · 2018-01-18T12:08:21Z

pymc3/distributions/distribution.py

Vaibhavdixit02 · 2018-01-19T05:36:56Z

@twiecki Could you please provide the link to the gist or line number of the test for Normal random method you mentioned, I couldn't find it?

twiecki · 2018-01-22T20:53:26Z

@Vaibhavdixit02 https://github.com/pymc-devs/pymc3/blob/718dab9ded5bf99d2fcfcd4ec8e2cf9769bb216a/pymc3/tests/test_distributions_random.py#L379

Vaibhavdixit02 · 2018-01-22T21:46:03Z

def test_densitydist(self):
        def ref_rand(size, mu, sd):
            return st.norm.rvs(size=size, loc=mu, scale=sd)
        normal_dist = pm.Normal.dist()
        pymc3_random(pm.DensityDist, {'logp':normal_dist.logp, 'random':normal_dist.random}, ref_rand=ref_rand)

This is the test I came up with, but it is throwing error when I run it locally. The error I got is,

distfam = <class 'pymc3.distributions.distribution.DensityDist'>, valuedomain = <pymc3.tests.test_distributions.Domain object at 0x7f4781f738d0>
vardomains = {'logp': <bound method Normal.logp of <pymc3.distributions.continuous.Normal object at 0x7f47125d3790>>, 'random': <bound method Normal.random of <pymc3.distributions.continuous.Normal object at 0x7f47125d3790>>}
extra_args = {}

    def build_model(distfam, valuedomain, vardomains, extra_args=None):
        if extra_args is None:
            extra_args = {}
        with Model() as m:
            vals = {}
            for v, dom in vardomains.items():
               vals[v] = Flat(v, dtype=dom.dtype, shape=dom.shape,
                               testval=dom.vals[0])
               AttributeError: 'function' object has no attribute 'dtype'

test_distributions.py:149: AttributeError

The problem is in the passing of functions (for logp and random) as parameters in the dictionary, I can't figure out any workaround for this can you help me out on this @twiecki?

twiecki · 2018-01-23T09:24:57Z

This is a bit more tricky than I thought. This is how far I got:

    def custom_random(self, point=None, size=None, repeat=None):
        mu, tau, _ = draw_values([self.mu, self.tau, self.sd],
                                 point=point)
        return generate_samples(stats.norm.rvs, loc=mu, scale=tau**-0.5,
                                dist_shape=self.shape,
                                size=size)

    def test_density_dist(self):
        def ref_rand(size, mu, sd):
            return st.norm.rvs(size=size, loc=mu, scale=sd)
        def create_custom_dens(name, mu=0, sd=1, shape=None, transform=None):
            return pm.DensityDist(name, lambda value: np.log(st.norm(loc=mu, scale=sd).pdf(value)), #logp
                random=custom_random)
        pymc3_random(create_custom_dens, {'mu': R, 'sd': Rplus}, ref_rand=ref_rand)

A couple of notes:

Seems like the test wants to be passed a class and create the RV itself (https://github.com/pymc-devs/pymc3/blob/718dab9ded5bf99d2fcfcd4ec8e2cf9769bb216a/pymc3/tests/test_distributions.py#L152). Here I'm passing a function that returns an object in the hope that it accepts it as well.
The random function also has a slightly more complex call signature. I copy&pasted the random method from pm.Normal and pass it into the created DensityDist but clearly this won't work as written. I would suggest to drop in there with a debugger like pdb++ (highly recommended to learn using this if you haven't already) and examine the arguments and how to return the right random numbers.

Hopefully that gets us a step closer.

Vaibhavdixit02 · 2018-01-23T19:35:01Z

Thank you for such a detailed reply, I will try what you have suggested and get back to you once I have some working version ready.

Vaibhavdixit02 · 2018-01-25T19:30:59Z

@twiecki are you familiar with any other similar test that could serve as a reference? My efforts have not lead to anything, I think looking at some example would be helpful.

junpenglao · 2018-01-27T10:11:30Z

@Vaibhavdixit02 implementing test could be tricky, you might find this PR useful: #2443
(As you can see from the discussion, I got stuck implementing the test at some point as well. You can have a look at the commit history, which might help as well)

Vaibhavdixit02 · 2018-01-30T09:56:38Z

Along the lines of the test for LKJCorr I tried.

    def test_density_dist(self):
        def ref_rand(size, mu, sd):
            return st.norm.rvs(size=size, loc=mu, scale=sd)
        
        class TestDensityDist(pm.DensityDist):

            def __init__(self, **kwargs):
                norm_dist = pm.Normal.dist()
                super(TestDensityDist, self).__init__('normal', logp=norm_dist.logp, random=norm_dist.random)

        pymc3_random(TestDensityDist, {},ref_rand=ref_rand)

But I am getting

    def __init__(self, **kwargs):
        norm_dist = pm.Normal.dist()
>       super(TestDensityDist, self).__init__('normal', logp=norm_dist.logp, random=norm_dist.random)
E       TypeError: __init__() got multiple values for keyword argument 'logp'

test_distributions_random.py:684: TypeError

Can't figure out why it says multiple arguments for logp

twiecki · 2018-01-30T10:55:32Z

Because the first argument is already the logp (https://github.com/pymc-devs/pymc3/pull/2805/files#diff-eb25725602c01ec0a384e4b6d3946331R181). You can just drop the 'normal' arg.

Vaibhavdixit02 · 2018-01-30T11:59:36Z

This test was passing locally for me, please tell me if there is some conceptual error. 🙂

twiecki · 2018-01-30T12:07:24Z

That looks great! Would it be easy from here to do the same put calling pm.DensityDist() directly?

Vaibhavdixit02 · 2018-01-30T12:21:08Z

The problem with pm.DensityDist() is that without a model instance calling it is not possible (like all other distributions) to instantiate it and my attempts of passing it without instantiating it failed as we need to pass the logp and random from the pm.Normal.dist() object.

twiecki · 2018-01-30T12:24:43Z

Oh it needs a model object on the stack? I suppose we could write a separate test that doesn't use the pymc3_random helper function and only tests that sampling works at all (i.e. not check the resulting distribution which is covered by this test). Sorry to be dense on this but I think we really should have a test in here that tests how a user would use it.

Vaibhavdixit02 · 2018-01-30T12:27:03Z

I think that would be the simplest and most straightforward approach, to create a separate model instance for DensityDist I am not sure how checking if the sampling is working fine would be implemented but over all this is the simpler approach.

twiecki · 2018-02-01T20:49:50Z

@Vaibhavdixit02 The test I'm imagining would not test that sampling is valid. It would literarily just be a function that creates a model context, creates a DensityDist with random, and then does ppc and makes sure there is no exception and the len(ppc_samples) > 0. Correctness is already established by the test you have now.

Vaibhavdixit02 · 2018-02-02T04:44:02Z

Okay sure. I think we could use the normal logp and random method in this test too?

twiecki · 2018-02-02T07:26:48Z

Definitely.

…

On Feb 2, 2018 05:44, "Vaibhav Kumar Dixit" ***@***.***> wrote: Okay sure. I think we could use the normal logp and random method in this test too? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2805 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AApJmHq6a9vnGQ5LjiviZLUJhYSARbg8ks5tQpKUgaJpZM4RigT1> .

Vaibhavdixit02 · 2018-02-02T09:19:29Z

@twiecki have added that test. 🙂

twiecki · 2018-02-02T09:22:18Z

pymc3/tests/test_distributions_random.py

Instead of returning we want to something like here: https://github.com/Vaibhavdixit02/pymc3/blob/216da86a6c8f9ae6516d133bc198ac45d678e716/pymc3/tests/test_distributions_random.py#L73 npt.assert_true(len(ppc) > 0, 'length of ppc sample is zero'). Check the np.testing submodule and other parts of the code for various usage patterns.

twiecki · 2018-02-02T09:23:02Z

pymc3/tests/test_distributions_random.py

This looks great. Does it work with a scipy distribution inside a lambda as well?

I didn't quite get that, can you elaborate on that a little?
My interpretation is something like

def mynormal_logp(): norm_dist_logp = st.norm.logpdf norm_dist_random = np.random.normal density_dist = pm.DensityDist('density_dist', norm_dist_logp, random=norm_dist_random)

is that what you meant?

I'll test this locally and check! BTW does PyMC3 handle scipy distributions in general?

twiecki · 2018-02-02T09:23:16Z

pymc3/tests/test_distributions_random.py

looks like a bad merge.

twiecki · 2018-02-02T09:23:57Z

This is shaping up nicely. See minor comments to resolve as well as the bad merge. Also, please add this (with credit) to the release-notes.

Vaibhavdixit02 · 2018-02-03T11:56:11Z

@twiecki added the test with scipy's normal distribution, the test passed locally but I am not very sure if it is correct, please provide feedback if it needs to be changed.

twiecki · 2018-02-04T19:16:01Z

This looks great, thanks for figuring this out!

twiecki · 2018-02-04T19:16:31Z

Only need to add this to the release notes.

twiecki · 2018-02-04T19:17:26Z

pymc3/tests/test_distributions_random.py

+
+            try:
+                ppc = pm.sample_ppc(trace, samples=500, model=model, size=100)
+                if len(ppc) > 0:


Just need to check if len(ppc) == 0

twiecki · 2018-02-04T19:17:40Z

pymc3/tests/test_distributions_random.py

+                normal_dist = pm.Normal.dist()
+                density_dist = pm.DensityDist('density_dist', normal_dist.logp, random=normal_dist.random)
+                step = pm.Metropolis()
+                trace = pm.sample(5000, step)


can do fewer steps here, e.g. 100 with tuning=0.

Vaibhavdixit02 · 2018-02-04T19:20:54Z

Should I add it to the release notes or will the maintainers do that?

twiecki · 2018-02-04T19:22:08Z

You should.

…

On Sun, Feb 4, 2018 at 8:20 PM, Vaibhav Kumar Dixit < ***@***.***> wrote: Should I add it to the release notes or will the maintainers do that? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2805 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AApJmHNtNrq_4sZLzFYWYUTXYv5x9X2aks5tRgMXgaJpZM4RigT1> .

twiecki · 2018-02-05T08:10:31Z

Congrats @Vaibhavdixit02!

Vaibhavdixit02 · 2018-02-05T08:15:05Z

Yay! 😄

twiecki · 2018-02-05T09:02:15Z

pymc3/distributions/distribution.py

    """Distribution based on a given log density function."""

-    def __init__(self, logp, shape=(), dtype=None, testval=0, *args, **kwargs):
+    def __init__(self, logp, shape=(), dtype=None, testval=0, random=None, *args, **kwargs):


Should add this to the doc-string.

twiecki · 2018-02-05T09:03:00Z

pymc3/tests/test_distributions_random.py

+            with model:
+                norm_dist_logp = st.norm.logpdf
+                norm_dist_random = np.random.normal
+                density_dist = pm.DensityDist('density_dist', normal_dist_logp, random=normal_dist_random)


I just realized, I think we need to turn this into an observed to actually get sampling from this.

twiecki · 2018-02-05T09:03:11Z

pymc3/tests/test_distributions_random.py

+
+            try:
+                ppc = pm.sample_ppc(trace, samples=500, model=model, size=100)
+                if len(ppc) == 0:


can remove this line.

twiecki · 2018-02-05T09:03:36Z

@Vaibhavdixit02 Sorry, I realized two more things here (see new comments). Could you also address those in a separate PR?

twiecki reviewed Jan 18, 2018

View reviewed changes

pymc3/distributions/distribution.py Outdated

Copy link

Member

twiecki Jan 18, 2018

Choose a reason for hiding this comment

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

I would move this to before *args to not mess with the order.

twiecki reviewed Jan 18, 2018

View reviewed changes

Vaibhavdixit02 force-pushed the editbranch1 branch from 5a6a174 to 8ecd3a3 Compare January 18, 2018 11:48

twiecki reviewed Jan 18, 2018

View reviewed changes

pymc3/distributions/distribution.py Outdated

Copy link

Member

twiecki Jan 18, 2018

Choose a reason for hiding this comment

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

PEP8 requires spaces after arguments

twiecki reviewed Jan 18, 2018

View reviewed changes

pymc3/distributions/distribution.py Outdated

Copy link

Member

twiecki Jan 18, 2018

Choose a reason for hiding this comment

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

same

junpenglao added this to the 3.4 milestone Jan 22, 2018

junpenglao added WIP enhancements labels Jan 27, 2018

Vaibhavdixit02 force-pushed the editbranch1 branch from 8ecd3a3 to b5ea182 Compare January 30, 2018 11:58

Vaibhavdixit02 force-pushed the editbranch1 branch from 187d12e to cae5c86 Compare February 2, 2018 05:09

Add random kwarg to DensityDist pymc-devs#2106

df52dc2

Vaibhavdixit02 force-pushed the editbranch1 branch from cae5c86 to 216da86 Compare February 2, 2018 05:38

twiecki reviewed Feb 2, 2018

View reviewed changes

pymc3/tests/test_distributions_random.py Outdated

Copy link

Member

twiecki Feb 2, 2018

Choose a reason for hiding this comment

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

looks like a bad merge.

Added test

905e83a

Vaibhavdixit02 force-pushed the editbranch1 branch from 216da86 to 905e83a Compare February 2, 2018 12:57

Test scipy distribution compatibility with DensityDist

38e14c1

twiecki reviewed Feb 4, 2018

View reviewed changes

Updated RELEASE-NOTES.md

e968bf7

twiecki merged commit 760fba4 into pymc-devs:master Feb 5, 2018

twiecki reviewed Feb 5, 2018

View reviewed changes

Conversation

Vaibhavdixit02 commented Jan 18, 2018

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!

twiecki commented Jan 18, 2018

Uh oh!

Vaibhavdixit02 commented Jan 18, 2018

Uh oh!

twiecki commented Jan 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vaibhavdixit02 commented Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

twiecki commented Jan 22, 2018

Uh oh!

Vaibhavdixit02 commented Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

twiecki commented Jan 23, 2018

Uh oh!

Vaibhavdixit02 commented Jan 23, 2018

Uh oh!

Vaibhavdixit02 commented Jan 25, 2018

Uh oh!

junpenglao commented Jan 27, 2018

Uh oh!

Vaibhavdixit02 commented Jan 30, 2018

Uh oh!

twiecki commented Jan 30, 2018

Uh oh!

Vaibhavdixit02 commented Jan 30, 2018

Uh oh!

twiecki commented Jan 30, 2018

Uh oh!

Vaibhavdixit02 commented Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

twiecki commented Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Vaibhavdixit02 commented Jan 30, 2018

Uh oh!

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

Uh oh!

Uh oh!

Vaibhavdixit02 commented Feb 2, 2018

Uh oh!

twiecki commented Feb 2, 2018 via email

Uh oh!

Vaibhavdixit02 commented Feb 2, 2018

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!

Vaibhavdixit02 Feb 2, 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!

twiecki commented Feb 2, 2018

Uh oh!

Vaibhavdixit02 commented Feb 3, 2018

Uh oh!

twiecki commented Feb 4, 2018

Uh oh!

Vaibhavdixit02 commented Jan 19, 2018 •

edited

Loading

Vaibhavdixit02 commented Jan 22, 2018 •

edited

Loading

Vaibhavdixit02 commented Jan 30, 2018 •

edited

Loading

twiecki commented Jan 30, 2018 •

edited

Loading

twiecki commented Feb 1, 2018 •

edited

Loading

Vaibhavdixit02 Feb 2, 2018 •

edited

Loading

twiecki Feb 4, 2018 •

edited

Loading