update BEST pymc-example to PyMC v4 #265
Conversation
- update to PyMC 4 - rename of greek variables spelling them - add header - update Autorhsip - add License section
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
View / edit / reply to this conversation on ReviewNB OriolAbril commented on 2022-01-09T09:57:42Z
I would also add some more tags, i.e.
You also need to fix the date to asuagar commented on 2022-01-12T19:12:11Z Following your recommendations:
|
|
View / edit / reply to this conversation on ReviewNB OriolAbril commented on 2022-01-09T09:57:43Z I don't know what you did here, but the notebook should be executed sequentially on a clean kernel (you can do restart and run all) and after that the code can't be modified.
If there were warnings about constrained layout when using seaborn plots, you can deactivate it here, then activate it right before starting to use arviz plots. Use
plt.rcParams["figure.constrained_layout.use"] = True/False asuagar commented on 2022-01-12T19:13:59Z I forgot to uncomment the line and run all on a clean kernel. Sorry for the oversight. I have done it now. OriolAbril commented on 2022-01-13T01:34:46Z the seaborn style should not be used. the arviz one is actually based on it and will look very similar, but the colour cycler of the arviz one is colorblind friendly whereas the one in seaborn is not. I think it's not critical in this particular notebook but it's much easier to review and enforce if we just apply it everywhere and also gives a feeling of coherence as if the notebooks were chapters of a single book instead of a random collection of pymc resources |
|
View / edit / reply to this conversation on ReviewNB OriolAbril commented on 2022-01-09T09:57:44Z Use PyMC only. This is confusing but from now on, the name of the library will be PyMC, if we wanted to specifically refer to its 4.x version we'd use PyMC 4.x, PyMC v4 or PyMC 4. PyMC4 was an experiment that was abandoned that tried to build pymc on top of tensorflow probability asuagar commented on 2022-01-12T19:16:04Z Ok. The text has been modified. Now it uses "PyMC" only. |
|
View / edit / reply to this conversation on ReviewNB OriolAbril commented on 2022-01-09T09:57:45Z This should not be added manually. You have to use the include directive as shown here: https://docs.pymc.io/en/latest/contributing/jupyter_style.html#epilogue asuagar commented on 2022-01-12T19:16:56Z The section "License" has been deleted. Now there is only the cell recommended in the link without any title. |
You have the syntax showing how to use references at https://docs.pymc.io/en/latest/contributing/jupyter_style.html#references. Jupyter can't render myst because cross-references, citations... require building all the files that comprise the documentation at the same time. However, jupyter renders only file per file. You can build the docs with sphinx following the guidance on https://github.com/pymc-devs/pymc-examples/blob/main/CONTRIBUTING.md#contributor-guide or push to the PR and wait for the readthedocs check (currently failing due to the date issue in the post directive) to generate a preview. |
OriolAbril
changed the title
- update to PyMC 4 - rename of greek variables spelling them - add header - update Autorhsip - add License section - use bib file for bibliography references
|
@asuagar you deleted the whole notebook in the latest commit |
|
Following your recommendations:
View entire conversation on ReviewNB |
|
I forgot to uncomment the line and run all on a clean kernel. Sorry for the oversight. I have done it now. View entire conversation on ReviewNB |
|
Ok. The text has been modified. Now it uses "PyMC" only. View entire conversation on ReviewNB |
|
The section "License" has been deleted. Now there is only the cell recommended in the link without any title. View entire conversation on ReviewNB |
|
Sorry for the mess. I have never done a GitHub commit using the console. It is being a headache trying to figure the right commands. I will try to make a new PR with the new files. Also, the "references.bib" file has been modified with the new references. |
|
@OriolAbril, you have to apologize my inexperience. I have done a new fork and updated two files (BEST.ipynb and references.bib). I do not know if the best option is (1) "Close with comment" this PR and make a new one or (2) doing other thing. I wait for your instructions. |
|
the seaborn style should not be used. the arviz one is actually based on it and will look very similar, but the colour cycler of the arviz one is colorblind friendly whereas the one in seaborn is not. I think it's not critical in this particular notebook but it's much easier to review and enforce if we just apply it everywhere and also gives a feeling of coherence as if the notebooks were chapters of a single book instead of a random collection of pymc resources View entire conversation on ReviewNB |
|
View / edit / reply to this conversation on ReviewNB OriolAbril commented on 2022-01-13T01:39:30Z I would use asuagar commented on 2022-01-14T14:54:45Z Updated. "{cite:t}" used as recommended. |
|
I added two extra nits now that it seems like the notebook is part of the PR again, but I can't find the notebook in the website preview: https://pymc--265.org.readthedocs.build/projects/examples/en/265/. Maybe it's best to open a new PR. Sorry you had issues with git and branches. Were the instructions on https://github.com/pymc-devs/pymc-examples/blob/main/CONTRIBUTING.md not clear enough? What could be changed to make things more clear? |
|
Fixed. "{cite:t}" as recommended. View entire conversation on ReviewNB |
|
The problem is not guide itself. It is exhaustive and precise. The issue is my low level of knowledge of Git and Github. I have never done something similar before. I will do a new PR. Sorry for the inconveniences. |
|
It is curious. The new commits are added to this PR. Would it be better to close this one and start other one? |
|
no need for a new PR. This has everything ready :). Here is the preview to the notebook: https://pymc--265.org.readthedocs.build/projects/examples/en/265/case_studies/BEST.html You'll see that references are missing, even though they are working in other examples. IIRC, bibtex keys are case sensitive, but it looks like the sphinx extension that simulates bibtex functionality doesn't find the citations when using uppercase letters. Can you try using only lowercase? Other than that everything looks great, thanks! |
|
I have updated the references.bib and the notebook for using lowercase references. I hope that it works. Thanks for your patience! |
There was a problem hiding this comment.
Everything looks great on the preview, waiting for someone else to double check there is nowhere in the code where we could take advantage of v4 features cc @twiecki @ricardoV94 maybe? Then we can merge
|
Thanks! |
update BEST pymc-example to PyMC4 #52
changes
potential improvements