Create guide for writing custom distributions #185

ally-lee · 2021-06-20T18:20:14Z

Description

Addresses issue #184 and aims to advance it to

review-notebook-app · 2021-06-20T18:20:18Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

pymc-devs/pymc#4775 (comment)

michaelosthege · 2021-07-30T18:06:07Z

examples/custom_distribution.ipynb

@@ -0,0 +1,425 @@
+{


And tests?

And maybe to consider contributing it with a PR ?

Reply via ReviewNB

michaelosthege · 2021-07-30T18:06:07Z

examples/custom_distribution.ipynb

@@ -0,0 +1,425 @@
+{


The next cell (implementation) should be introduced by the text. For example, that it should be an implementation of the above formula using just Aesara operations.

Reply via ReviewNB

I find confusing that it uses np.log and tt.switch

examples/custom_distribution.ipynb

michaelosthege · 2021-07-30T18:06:08Z

examples/custom_distribution.ipynb

@@ -0,0 +1,425 @@
+{


With https://github.com/pymc-devs/pymc/pull/4867 we're adding a mechanism for setting initial values. The code example doesn't need to include that, but maybe it should be mentioned somewhere.

Reply via ReviewNB

Is this guide supposed to be for 3.x or for 4.x as of now?

I believe I'm still working with 3.x

examples/custom_distribution.ipynb

michaelosthege · 2021-07-30T18:06:08Z

examples/custom_distribution.ipynb

@@ -0,0 +1,425 @@
+{


Line #3. chains=2, cores=1, init='adapt_diag', random_seed=42)
For v4 the typical variable name would be idata for the returned InferenceData object.

Reply via ReviewNB

even if using v3, all notebooks should already use return_inferencedata=True, and if possible use coords and dims instead of shape.

michaelosthege · 2021-07-30T18:06:08Z

examples/custom_distribution.ipynb

@@ -0,0 +1,425 @@
+{


pm.traceplot is deprecated in favor of either arviz.plot_trace or its alias pm.plot_trace.

Reply via ReviewNB

examples/custom_distribution.ipynb

michaelosthege · 2021-07-30T18:06:58Z

@ally-lee nice work! I left some comments of which some may be nitpicky.

I really like the practical application example!!

examples/custom_distribution.ipynb

OriolAbril · 2021-08-04T14:44:32Z

examples/custom_distribution.ipynb

@@ -0,0 +1,425 @@
+{


Thinking in terms of https://github.com/pymc-devs/pymc/issues/4899 (so it doesn't need to be done in this PR, we can add this to the issue and nothing else). pmf/pdf maybe also over/underdispersion look like terms that should go in the glossary.

Reply via ReviewNB

Would it be helpful if I added these terms/definitions in a comment on that issue?

That would be great yes! Thanks!

examples/custom_distribution.ipynb

OriolAbril · 2021-08-04T14:46:52Z

This looks great, sorry it took so long to review. I have added some comments related to latest updates and best practices with pymc3 and arviz which will require some changes to the plotting. I am happy to help with those changes

ally-lee · 2021-08-08T01:49:16Z

@michaelosthege @OriolAbril Thank you both for your reviews! I made a bunch of updates based on your comments. Do you mind taking it over to make the more technical changes? I think that would be best since you're the experts on this technology. I'm feeling good about what I've submitted so far and am okay with whatever changes you need to make

ricardoV94 · 2021-08-08T05:35:50Z

examples/pymc3_howto/custom_distribution.ipynb

@@ -0,0 +1,471 @@
+{


There are two extra details that are worth mentioning:

For continuous distributions you also have to define the default transform, or inherit from a more specific class like PositiveContinuous which specifies what the default transform should be.
On V3 you are supposed to also specify at least one ”default value" for the distribution during init such as self.mode, self.median and self.mean (the latter only for continuous distributions). This is used by some samplers or other compound distributions. It didn't show up in your case because you are not sampling from the distribution (as it it's observed)

Reply via ReviewNB

Thank you for pointing these out! I'll add these notes to the text block. I also realized the reason I needed to set a testval when sampling was because I hadn't specified a "default value" for the distribution, so I can take that out now.

ricardoV94 · 2021-08-08T05:45:57Z

examples/pymc3_howto/custom_distribution.ipynb

@@ -0,0 +1,471 @@
+{


I would suggest adding a prior predictive step, not only because it's part of the standard Bayesian worflow, but also because it uses your distribution in a different way than in sample or posterior_predictive, as an unobserved random variable. That in itself of a good sanity check that the distribution is well implemented and can be sampled from (with mcmc that is)

Reply via ReviewNB

I'm trying to add this, but I think it won't work because pm.AR doesn't have a random method. Is it ok if I leave this step out?

Actually what I had in mind was prior predictive sampling with pm.sample by simply removing the observed keyword.

But that's not necessarily useful for this example...

Feel free to ignore it then

I like your idea of doing a sanity check though! I think it would be nice to compare what samples look like from the standard Poisson vs generalized Poisson

That's a good point!

ally-lee · 2021-08-14T19:25:20Z

Hey @OriolAbril just wanted to check in, are you ok to make those final changes related to latest updates/best practices? Is there anything else I can do to help get this ready to release?

chiral-carbon · 2021-08-18T07:08:49Z

hey @ally-lee, I would be up for making the final changes to get this notebook to best practices, if that's alright with you?
@MarcoGorelli would that be okay?

MarcoGorelli · 2021-08-18T08:11:45Z

sure, no objections from me

chiral-carbon · 2021-08-18T19:48:22Z

@MarcoGorelli would you merge this notebook then, if everything else (except using return_inferencedata=True in pm.sample and plotting using ArviZ) seems alright? because then I could open a new PR with the new changes.

MarcoGorelli · 2021-08-18T19:50:34Z

@MarcoGorelli would you merge this notebook then, if everything else (except using return_inferencedata=True in pm.sample and plotting using ArviZ) seems alright? because then I could open a new PR with the new changes.

I haven't had a chance to look at this yet, but you could always make a new branch from this one so as to keep @ally-lee 's commits and to add your ones on top

ally-lee · 2021-08-26T17:40:14Z

Hey @chiral-carbon thanks for picking this up! Just curious if you have a status update on this

chiral-carbon · 2021-08-27T10:16:16Z

hey @ally-lee I will be able to push some commits in a day or two. hope that's alright!

OriolAbril

I think it's better to merge this PR so that then @chiral-carbon can work on the notebook and open a new PR directly to main. This will avoid PRs on PRs which I don't like by no reason at all and given that we squash merge PRs here, will leave two commits related to this notebook in the history, one by @ally-lee and one by @chiral-carbon

Unless someone opposes I'll merge tomorrow

chiral-carbon · 2021-08-27T21:01:49Z

I made a few changes to this NB by adding allylee's remote locally and creating a new branch but I could always save that notebook and just move it so no problem by me @OriolAbril

OriolAbril · 2021-08-28T08:54:33Z

Then it's probably easier for you to open the PR to this branch/PR I think 🤔

chiral-carbon · 2021-08-28T11:21:56Z

either is fine tbh. I would also have doubts and probably make some changes incrementally, so if having lots of comments under this one PR would not be too messy then I'll just push here. otherwise, I can open a new PR, no issues there.

chiral-carbon · 2021-08-31T15:35:41Z

hey @ally-lee I tried pushing commits to this PR but I get a git error saying permission denied. could you check if the option "Allow edits from maintainers" is checked in the PR? it should be a checkbox on the right side of this page!

ally-lee · 2021-08-31T20:22:50Z

Hmm it's already checked

chiral-carbon · 2021-09-01T08:07:01Z

oh okay 🤔 not sure what's wrong then

MarcoGorelli · 2021-09-01T08:36:15Z

oh okay 🤔 not sure what's wrong then

can you show the command you ran, and the output?

OriolAbril · 2021-09-01T08:40:20Z

I think the problem is that @chiral-carbon only has triage permissions so she isn't seen as "maintainer" by github

chiral-carbon · 2021-09-01T10:29:15Z

@OriolAbril @MarcoGorelli I searched online and I think only the owners/maintainers of a project can push commits to a PR opened by someone else in the project, or if I had write access to @ally-lee's fork then also it might work.
this is the error message I get on doing git push allylee HEAD:custom-distribution ( I had added @ally-lee's fork as a remote locally with git remote add allylee https://github.com/ally-lee/pymc-examples.git):

remote: Permission to ally-lee/pymc-examples.git denied to chiral-carbon.
fatal: unable to access 'https://github.com/ally-lee/pymc-examples.git/': The requested URL returned error: 403

will you consider merging this PR in that case? and I'll open a new one?

MarcoGorelli

Sure, looks good to me - I've finally had a chance to read through the notebook, and I think it's really good!

Create custom_distribution.ipynb

98552cc

ally-lee added 3 commits June 20, 2021 11:25

Update custom_distribution.ipynb

ec5a6af

Replace 0 with -inf

71c5738

Set bound on theta to be > 0

92158c3

ricardoV94 mentioned this pull request Jun 23, 2021

Add V4 distribution implementation developer guide pymc-devs/pymc#4783

Merged

Simplify logp expression

90d89c4

pymc-devs/pymc#4775 (comment)

michaelosthege reviewed Jul 30, 2021

View reviewed changes

OriolAbril reviewed Aug 4, 2021

View reviewed changes

ally-lee added 6 commits August 7, 2021 17:46

address PR comments

855c914

upload csv to data folder

94449cb

move notebook to examples/pymc3_howtos

2398a9a

add notebook to table of contents

5c7a62c

read csv from data folder using try except clause

d885457

pre-commit

211d4f2

ricardoV94 reviewed Aug 8, 2021

View reviewed changes

ally-lee added 3 commits August 8, 2021 11:23

address PR comments

3008a47

make predictions on heldout set instead of future

1f08bb4

add sampling sanity check

f10571b

ally-lee mentioned this pull request Aug 14, 2021

Glossary terms pymc-devs/pymc#4899

Closed

OriolAbril approved these changes Aug 27, 2021

View reviewed changes

MarcoGorelli approved these changes Sep 1, 2021

View reviewed changes

MarcoGorelli merged commit 2002ebd into pymc-devs:main Sep 1, 2021

ricardoV94 mentioned this pull request Apr 20, 2022

Review potentially outdated docs pymc-devs/pymc#5538

Closed

4 tasks

Create guide for writing custom distributions #185

Create guide for writing custom distributions #185

Conversation

ally-lee commented Jun 20, 2021

Description

review-notebook-app bot commented Jun 20, 2021

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

michaelosthege commented Jul 30, 2021 • edited Loading

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

OriolAbril commented Aug 4, 2021

ally-lee commented Aug 8, 2021

ricardoV94 Aug 8, 2021 • edited Loading

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

ally-lee commented Aug 14, 2021

chiral-carbon commented Aug 18, 2021

MarcoGorelli commented Aug 18, 2021

chiral-carbon commented Aug 18, 2021

MarcoGorelli commented Aug 18, 2021

ally-lee commented Aug 26, 2021

chiral-carbon commented Aug 27, 2021

OriolAbril left a comment

Choose a reason for hiding this comment

chiral-carbon commented Aug 27, 2021

OriolAbril commented Aug 28, 2021

chiral-carbon commented Aug 28, 2021

chiral-carbon commented Aug 31, 2021

ally-lee commented Aug 31, 2021

chiral-carbon commented Sep 1, 2021

MarcoGorelli commented Sep 1, 2021

OriolAbril commented Sep 1, 2021

chiral-carbon commented Sep 1, 2021 • edited Loading

MarcoGorelli left a comment

Choose a reason for hiding this comment

michaelosthege commented Jul 30, 2021 •

edited

Loading

ricardoV94 Aug 8, 2021 •

edited

Loading

chiral-carbon commented Sep 1, 2021 •

edited

Loading