Skip to content

[DOC] PyMC, Aesara and Aeppl intro notebook #5721

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 33 commits into from
Jun 6, 2022
Merged

[DOC] PyMC, Aesara and Aeppl intro notebook #5721

merged 33 commits into from
Jun 6, 2022

Conversation

juanitorduz
Copy link
Contributor

This PR partially solves #5538.

I am trying to add comments and understand @ricardoV94's nice intro notebook. Nevertheless, I still have a long way to fully understand. Specifically, starting from section Graph manipulation 102 things become less clear for me. Still, as suggested I am opening this PR to get feedback and iterate.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@juanitorduz juanitorduz changed the title [DOC] [DOC] PyMC, Aesara and Aeppl intro notebook Apr 20, 2022
@juanitorduz
Copy link
Contributor Author

@ricardoV94 What do you thing should be the scope of this notebook? Specifically, which sections we should focus on and which ones we should remove (or postpone)?

@juanitorduz juanitorduz marked this pull request as draft April 20, 2022 18:23
@ricardoV94
Copy link
Member

ricardoV94 commented Apr 20, 2022

@ricardoV94 What do you thing should be the scope of this notebook? Specifically, which sections we should focus on and which ones we should remove (or postpone)?

For now you can focus on PyMC returning RandomVariables (you can skip most of the Aesara Op and specially the manual seeding bit), and the "vanilla" logprob part (you can skip the Aeppl bits as well). What's left would be the most critical part

@juanitorduz
Copy link
Contributor Author

@ricardoV94 in 54b2e69 I removed the parts of the notebook which are not needed based in your input. Would you agree that the current content is what I need to develop? I think I would have some time these upcoming weeks to work on this notebook (at least some times during the evenings)

Two small remarks on code errors (which you can see on the notebook):

  • In my current version (4.b06) I am getting AttributeError: 'Model' object has no attribute 'compute_initial_point'
  • At the beginning of the section What is the deal with those value variables in the model? I whet an error because rv is not defined, what should rv be?

Thanks 🙏 !

@OriolAbril
Copy link
Member

In my current version (4.b06)

I can't speak about the method in question, but you should not be executing that notebook on a release and instead do that on the development version aka local aka from github main. This notebook will be part of the versioned docs and will be re-executed every time the docs are built (so it can't take very long to run either).

I'll try to review the formatting, style and cross-references of the notebook some time next week.

@juanitorduz
Copy link
Contributor Author

@OriolAbril so this means I should use the local development environment described in https://github.com/pymc-devs/pymc/wiki/Installation-Guide-(MacOS)#pymc-v4-installation-on-macos ? This way I am installing from the main branch?

@OriolAbril
Copy link
Member

That looks right. Just to be sure, you'll find better guidance and avise on the contributing section of the documentation (when reading contributing docs always do so on the "latest" version/tag): https://docs.pymc.io/en/latest/contributing/index.html.

@ricardoV94
Copy link
Member

ricardoV94 commented Apr 23, 2022

Would you agree that the current content is what I need to develop?

Yes it looks about right. And I love the changes you already added!

Two small remarks on code errors (which you can see on the notebook):

  • In my current version (4.b06) I am getting AttributeError: 'Model' object has no attribute 'compute_initial_point'

You might have already figured it out, but we renamed it to initial_point to be more succint.

At the beginning of the section What is the deal with those value variables in the model? I whet an error because rv is not defined, what should rv be?

I'll have to check but I probably reordered some part and didn't fix it. It was a scipy frozen rv like

import scipy.stats
rv = scipy.stats.norm(mu, sigma)

With whatever values for mu and sigma make sense

@ricardoV94 ricardoV94 mentioned this pull request Apr 26, 2022
4 tasks
@twiecki
Copy link
Member

twiecki commented Jun 3, 2022

Can we make an abridged version here that we can merge and then improve it in subsequent PRs?

@juanitorduz juanitorduz requested a review from ricardoV94 June 3, 2022 10:58
@juanitorduz
Copy link
Contributor Author

juanitorduz commented Jun 3, 2022

@ricardoV94 Sorry about coming back to this notebook so late but I have been quite busy 😓 ! Today I revisited it and cleaned and extracted the most relevant pars of the initial notebook. I could see myself working in this notebooks for days but in view of @twiecki 's comment about having an initial version to iterate with my questions are:

  • What are the essentials parts to be added or removed?
  • Any feedback for the first iteration?

As you have a better overview of the documentation, your input about how to make this notebook useful and not redundant would be great! Based on you input I will work to ship this one (at least the first iteration) soon 🙏 .

@@ -0,0 +1,1819 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

I'd include an example that actually works. Otherwise I wouldn't even expect it to work because it's not a RV directly.


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any particular suggestion on an interesting example?

Copy link
Member

Choose a reason for hiding this comment

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

I'm actually surprised it doesn't work with cumsum(). @ricardoV94 would know.

@twiecki
Copy link
Member

twiecki commented Jun 3, 2022

Minor comments but this looks great!

@twiecki twiecki marked this pull request as ready for review June 3, 2022 11:41
@juanitorduz
Copy link
Contributor Author

Thanks for all your feedback! After addressing all comments (I hope) and a crash course on sphinx, it would be great if you can give me some final feedback. Thanks again.

@ricardoV94
Copy link
Member

@juanitorduz I pushed a commit that shuffles a small portion of the guide around to present pm.logp before introducing the Model.logpt and Model.compile_logp methods. What do you think? Feel free to revert or modify if you don't like the changes. Also I wouldn't be surprised if I screw up the new sphinx references, so apologies in advance for that!

@ricardoV94
Copy link
Member

ricardoV94 commented Jun 6, 2022

Does the reviewnb not show difference per-commit or did I screw up something? Seems like the latter.

@OriolAbril
Copy link
Member

Does the reviewnb not show difference per-commit

it does not. I skimmed the current preview and it looks good, we can continue to iterate and improve it further if needed. There are some references that are not working. I think they are wrong right now, but they also can't work. The root of the issue is #5282.

We are using pm.logp in the code, so the reference should be {func}`~pymc.logp`. However, that won't work because pymc.logp is not documented anywhere. It is fine to leave the references with the right syntax and not generating the correct links because once we document them they will be generated and point to the right place, it is not right to leave pymc.distributions.logprob... as references

@OriolAbril
Copy link
Member

Note, I was going to comment on the issue that the logp functions are missing but it is already there: #5282 (comment)

@OriolAbril
Copy link
Member

Yes, references should always be the same as what was used in the code (with maybe adding the ~ before it so shorten the link text) and api pages should always document objects at that path. The file path where the object is implemented is irrelevant to users

@ricardoV94
Copy link
Member

Yes, references should always be the same as what was used in the code (with maybe adding the ~ before it so shorten the link text) and api pages should always document objects at that path. The file path where the object is implemented is irrelevant to users

I removed my comment, because I realized you had already answered :D

@juanitorduz
Copy link
Contributor Author

Thanks for all the improvements 🚀 ! In 79212ef I changes the references of the functions which are not yet in the docs, but of course we want to have them right.

Anything else missing? (I might be slower to contribute during the working day so feel free to add / remove parts in the meantime)

@ricardoV94
Copy link
Member

Found a typo, otherwise looks 👌

@twiecki twiecki merged commit da1f63b into pymc-devs:main Jun 6, 2022
@twiecki
Copy link
Member

twiecki commented Jun 6, 2022

This is awesome -- thanks @juanitorduz!

@juanitorduz juanitorduz deleted the docs/aesara_pymc_nb branch June 7, 2022 08:30
@twiecki
Copy link
Member

twiecki commented Jun 7, 2022

@OriolAbril how do we add this to the website?

@ricardoV94
Copy link
Member

ricardoV94 commented Jun 7, 2022

It's here: https://www.pymc.io/projects/docs/en/latest/learn/core_notebooks/pymc_aesara.html

One needs to switch the docs version to latest when coming from the main docs page

@OriolAbril
Copy link
Member

the notebooks from pymc examples and the gallery page gets updated automatically with every commit. But this notebook is not part of pymc-examples, it is part of the small subset of notebooks on core notebooks we keep here in the versioned docs. These are re-executed every time we build the docs and will always be in sync with our releases (a change in the library that breaks any of the code in these notebooks will break ci)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants