pymc-examples
pymc-examples copied to clipboard
pymc-devs
Update pymc3_howto/custom_distribution NB to best practices
Aims to advance https://github.com/pymc-devs/pymc-examples/issues/184 to best practices.
Check out this pull request on 
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB
![review-notebook-app[bot] avatar](https://avatars.githubusercontent.com/in/15474?v=4 "Published by a pub.dev verified publisher"){kind=small}
View / edit / reply to this conversation on ReviewNB
OriolAbril commented on 2021-09-08T16:34:45Z ----------------------------------------------------------------
not sure what is the syntax in the priors nor how we'd like that to look like, but there may be extra dollars or something of the sort. Maybe it's because the math expressions should be on their own line as block math instead of inline.
chiral-carbon commented on 2021-09-08T17:14:02Z ----------------------------------------------------------------
it renders without the dollar sign though in jupyter when I see it locally. will remove the expression from being inline and push again
View / edit / reply to this conversation on ReviewNB
OriolAbril commented on 2021-09-08T16:34:46Z ----------------------------------------------------------------
this uses bias as beta0 and beta_recent as beta1. the equivalences are also in the prior bullet points, but I think it would be better to use the same names everywhere.
The coords also need some extra work. I am not sure if the csv contains the actual dates, in which case we could use that, but we'll need 3 "dimensions": total_days, observed_days and future/holdout_days. y_past should be annotated as observed_days dims
View / edit / reply to this conversation on ReviewNB
OriolAbril commented on 2021-09-08T16:34:47Z ----------------------------------------------------------------
This is not working correctly. you'll need to use from_pymc3 or from_pymc3 predictions to get chain and draw dims right. Otherwise, you'll need to use the keep_size=True and pass all the dimensions and coords manually anew. I think you already fixed that same issue in another notebook.
also, annotate y_future 's dimensions, you'll already have the coords in the model object if you followed my previous recommendation
View / edit / reply to this conversation on ReviewNB
OriolAbril commented on 2021-09-08T16:34:48Z ----------------------------------------------------------------
Line #9. draw=slice(i, None, None)
this is doing the same the previous code did, but it's nominally wrong, this dimension should not be the draw one but the one of the time ones
once all that is fixed, the loop is not really necessary if we have the xarray dataset correctly labeled. the code could be modified to
qs = trace.posterior_predictive["y_future"].quantile((0.025, .5, .975), dim=("chain", "draw"))
low = qs.sel(quantile=0.025)
median = qs.sel(quantile=0.5)
...
it renders without the dollar sign though in jupyter when I see it locally. will remove the expression from being inline and push again
View entire conversation on ReviewNB
it renders without the dollar sign though in jupyter when I see it locally. will remove the expression from being inline and push again
jupyter, reviewnb and sphinx (via myst) are 3 different ways of rendering notebooks. For our purposes here, what matters is sphinx rendering to work properly. Reviewnb and jupyter should be the same rendering, but sphinx is by design a completely different paradigm as it doesn't render notebooks independently but as a whole and it renders them into a website using extensions and a theme.
the references don't look right, you should double check the bibtex source (you can use google scholar to generate it for you). Right now some have 3 somewhat repeated links but not all of them work
I used the bibtex citations provided by the website of the paper themselves, using the doi links does open the correct website so I'm not sure what to change
View / edit / reply to this conversation on ReviewNB
OriolAbril commented on 2021-10-02T12:23:20Z ----------------------------------------------------------------
the expectance of Y formula is not rendered as block math but as inline math instead
View / edit / reply to this conversation on ReviewNB
OriolAbril commented on 2021-10-02T12:23:20Z ----------------------------------------------------------------
Do not use sanity check, here are some alternatives: https://gist.github.com/seanmhanson/fe370c2d8bd2b3228680e38899baf5cc
@chiral-carbon what is the status of this? Is it close to done, or should I close it?
