Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@tjburch Thank you for doing this! The plots look great and I’m sure the maintainers will be pleased that someone took care of this.

thanks for the PR! I don't have time to review now but wanted to leave two quick comments. Can you also add a link in the description to the relevant tracking issue? #234 And add a post directive to the notebook so it can be found via tags? https://github.com/pymc-devs/pymc/wiki/PyMC-Jupyter-Notebook-Style-Guide#post-directive

@OriolAbril - added the link, will get to the post directive ASAP. Traveling the next couple of days so might be Tuesday before I get to it, sorry for the delay.

Added in latest commit

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-10-13T04:07:55Z
----------------------------------------------------------------

All pymc classes used should be added as tags. If you install pre-commit, it will automatically update the tags with all classes used (in a similar way, it checks that all classes are present as tags, now that I have approved and run the CI checks you can see it's failing because of that). I would also use patsy only as tag instead of the specific function. There are a couple examples that use patsy and I think it's good to have the tag and to show it can be integrated with ArviZ but I think we should only use functions/classes for pymc things, for external libraries I think it's too specific.

tjburch commented on 2021-10-13T23:47:03Z
----------------------------------------------------------------

Done. Just did classes, not methods (e.g. not pm.sample_prior_predictive) following other notebooks.

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-10-13T04:07:56Z
----------------------------------------------------------------

I would make this use pm.get_data("cherry_blossoms.csv") as suggested in https://github.com/pymc-devs/pymc/wiki/PyMC-Jupyter-Notebook-Style-Guide#reading-from-file. The only difference is getting the file from rethinking repo or from pymc-examples one when the file is not found locally and the rethinking might change it's contents with new editions for example, whereas changes here will also be synced with the notebook

tjburch commented on 2021-10-13T23:47:19Z
----------------------------------------------------------------

Done.

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-10-13T04:07:56Z
----------------------------------------------------------------

what do you think about hiding this output so that it can be toggled if desired but hidden by default? I'm not sure seeing all this text will mean anything to users, especially if targetting the notebook to beginners.

reference: https://myst-nb.readthedocs.io/en/latest/use/hiding.html#hiding-code-cells, https://jupyterbook.org/content/metadata.html and https://discourse.jupyter.org/t/how-to-edit-cell-tags-in-jupyter-lab/4419

tjburch commented on 2021-10-13T23:49:18Z
----------------------------------------------------------------

I think I did this correctly, let me know if not. Output still showing in the github rendering and reviewnb.

OriolAbril commented on 2021-10-14T00:59:43Z
----------------------------------------------------------------

it is supposed to still appear in both github and reviewnb, it is when rendered into the html docs with sphinx that the toggle thing is taken into account.

As you can see from the preview: https://pymc-examples--238.org.readthedocs.build/en/238/splines/spline.html (also accessible via the checks in the PR, click on details and search for splines for example to find the notebook) the toggle thing is working.

What is not working are the tags, you'll see that splines or patsy don't appear. Please make sure that the directive is on the first cell and that each colon fence and metadata field are on their own line just like in the example: https://github.com/pymc-devs/pymc/wiki/PyMC-Jupyter-Notebook-Style-Guide#post-directive

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-10-13T04:07:57Z
----------------------------------------------------------------

Line #7.    color = plt.cm.rainbow(np.linspace(0, 1, len(spline_df.spline_i.unique())))

can you try a different colormap? I have a bit of trouble seeing colors 7-10

tjburch commented on 2021-10-13T23:51:25Z
----------------------------------------------------------------

Tough to find a colormap that doesn't include yellow to some effect. Went with one that covered a few colors, and then iterated over the space of the colormap that didn't include yellow (so linspace now runs to 0.8. I think it gets the point across, but it's kind of annoying that 4 or so of these are black now.

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-10-13T04:07:58Z
----------------------------------------------------------------

can you use named coords and dims to define the shapes?

tjburch commented on 2021-10-13T23:52:03Z
----------------------------------------------------------------

Done. Haven't done a ton of models this way yet, but I think it's correct, let me know if otherwise.

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-10-13T04:07:58Z
----------------------------------------------------------------

can you add a cell with watermark as title instead? this way it will appear in the toctree on the right sidebar in case users want to check it out before reading/running the notebook.

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-10-13T04:07:59Z
----------------------------------------------------------------

patsy doesn't appear even though it's imported, can you add it manually like with xarray and theano?

Thanks! I have commented a few nits, nothing too significant

It is also great that you have added yourself to the notebook, like @jhrcook did. As not everybody does, we were thinking about adding that the the style guide and setting a common style for it, both of you for example have not linked to your github profile now website like other authors do. If you can it would be great to have your opinions and preferences at #198 which is where we are discussing this.

Done. Just did classes, not methods (e.g. not pm.sample_prior_predictive) following other notebooks.

View entire conversation on ReviewNB

Done.

View entire conversation on ReviewNB

I think I did this correctly, let me know if not. Output still showing in the github rendering and reviewnb.

View entire conversation on ReviewNB

Tough to find a colormap that doesn't include yellow to some effect. Went with one that covered a few colors, and then iterated over the space of the colormap that didn't include yellow (so linspace now runs to 0.8. I think it gets the point across, but it's kind of annoying that 4 or so of these are black now.

View entire conversation on ReviewNB

Done. Haven't done a ton of models this way yet, but I think it's correct, let me know if otherwise.

View entire conversation on ReviewNB

It is also great that you have added yourself to the notebook, like @jhrcook did. As not everybody does, we were thinking about adding that the the style guide and setting a common style for it, both of you for example have not linked to your github profile now website like other authors do. If you can it would be great to have your opinions and preferences at #198 which is where we are discussing this.

Made a comment over on #198 that you've already seen. I went ahead and adapted the authorship comments here to reflect the 3rd option mentioned on that issue.

View / edit / reply to this conversation on ReviewNB

tjburch commented on 2021-10-14T00:16:27Z
----------------------------------------------------------------

It's unclear to me why this isn't rendering in reviewnb as a title, despite being a md cell with ## in front. Any thoughts here?

OriolAbril commented on 2021-10-14T01:06:07Z
----------------------------------------------------------------

reviewnb can sometimes be a bit buggy. It is working on the preview: https://pymc-examples--238.org.readthedocs.build/en/238/splines/spline.html?highlight=spline#watermark

it is supposed to still appear in both github and reviewnb, it is when rendered into the html docs with sphinx that the toggle thing is taken into account.

As you can see from the preview: https://pymc-examples--238.org.readthedocs.build/en/238/splines/spline.html (also accessible via the checks in the PR, click on details and search for splines for example to find the notebook) the toggle thing is working.

What is not working are the tags, you'll see that splines or patsy don't appear. Please make sure that the directive is on the first cell and that each colon fence and metadata field are on their own line just like in the example: https://github.com/pymc-devs/pymc/wiki/PyMC-Jupyter-Notebook-Style-Guide#post-directive

View entire conversation on ReviewNB

reviewnb can sometimes be a bit buggy. It is working on the preview: https://pymc-examples--238.org.readthedocs.build/en/238/splines/spline.html?highlight=spline#watermark

View entire conversation on ReviewNB

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-10-14T01:07:54Z
----------------------------------------------------------------

cool! I would also add a title of level 2 like in references or watermark so at least from the top there is the authors section to draw a bit of attention here. And I'll also look into the sidebar thing, thanks for commenting on the issue by the way

Thanks for the info on that, sorry for the post-directive error - fixed that and added the Authors title.

Thanks for the PR @tjburch !