Add start of dimensionality notebook doc #5437

canyon289 · 2022-02-01T05:18:02Z

Adding start of shapes doc. Note for this PR I am not intending to reach "feature exhaustive" but more minimally useful that can be built upon.

There's two primary areas I could use with in this PR in ranked order

Feedback on the notebook to ensure the documentation is correct and useful
Help getting it to actually show up in the rendered html. The things Ive tried so far haven't seemed to work

Huge thanks to @michaelosthege who spent 2 hours in his Friday evening explaining this to me very patiently and kindly

review-notebook-app · 2022-02-01T05:18:06Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

docs/source/learn/examples/dimensionality.ipynb

codecov · 2022-02-01T05:25:52Z

Codecov Report

Merging #5437 (e4e80b7) into main (3d958ad) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #5437   +/-   ##
=======================================
  Coverage   80.23%   80.23%           
=======================================
  Files          82       82           
  Lines       13941    13941           
=======================================
  Hits        11185    11185           
  Misses       2756     2756

docs/source/learn/examples/dimensionality.ipynb

ricardoV94 · 2022-02-01T07:15:38Z

docs/source/learn/examples/dimensionality.ipynb

@@ -0,0 +1,709 @@
+{


Reiterate this is the "implied" situation again?

Reply via ReviewNB

A general advice regarding shape, probably worth adding at the top: __Pick prime numbers for dimension lenghts.__
Then one can always reconstruct how, for example, two variables were concatenated.
Also makes it easier to pinpoint errors.

In this example, using eye(3) would make it clear which output dimensions correspond to the support dim.

I dont quite get what you mean here, though I took a guess and added a new cell to show the limit of where Im losing the understanding

What Michael meant is that instead of having a (2, 2) distribution (repeated dimensions), you should use something like (2, 3), so that it's obvious what is base and what is implied/replicated dimensionality.

Your example now needs a size 3 vectors for mu though. That's why it's failing

docs/source/learn/examples.md

OriolAbril · 2022-02-01T19:21:48Z

also, more of a PSA, I am not subscribed to notifications for pymc repo, only for pymc-examples. If there are docs PR please tag me so I see them asap. I try to review new issues and PRs 1-2 a week and I saw this one relatively early, but it is often too late already

docs/source/learn/examples/dimensionality.ipynb

canyon289 · 2022-02-02T04:33:12Z

also, more of a PSA, I am not subscribed to notifications for pymc repo, only for pymc-examples. If there are docs PR please tag me so I see them asap. I try to review new issues and PRs 1-2 a week and I saw this one relatively early, but it is often too late already

Noted @OriolAbril, sorry about that and thanks for reviewing

ricardoV94 · 2022-02-02T05:35:41Z

docs/source/learn/examples/dimensionality.ipynb

@@ -0,0 +1,742 @@
+{


mu=[0, 0]

Technically, it's incorrect to specify a scalar mu for the multivariate normal, although for backwards compatibility we reshape mu behind the scenes

Reply via ReviewNB

If its just for backwards compability why dont we drop it?

We can drop things after warning about them for long enough. If you want to drop supporting the automatic broadcast, then we'll need FutureWarning in a release version for about a year. After that we can make the break and argue that it's okay to not bump the major version.

But we have a major release coming up where breaks are expected.

We can drop it with a useful ValueError about the changed behavior. See also #5440

I agree with Thomas, we're releasing a major version this is our chance to reform the code without the need for FutureWarnings.

This is good for both devs and the community. For example in this case some of these incorrect but sort of works situations makes using the library more challenging. As a dev when I try and figure out what's happening I have to navigate the maze of conditionals and reference calls in the backend to figure out what is happening. I also need to read through so many much more code and tests to make a useful contribution.

In the nicest way I'm finding you (Ricardo and Michael) are PyMC/Aesara/AePPL power users and you're not inside the maze of code. You guys are so smart you have this elevated view and can see this maze from above, avoid all the traps, and know all the shortcuts. For noobs like me I run into all the deadends and then have to backtrack and try another route.

My strong pitch is we make the maze simpler. As with all codebases the complexity will increase over time, look at Theano. But we have this one opportunity right to make things simpler so please lets please do it, if not for you for me ❤️

This is a bigger discussion than this PR so if you have thoughts let move it to more conversational medium.

In the meanwhile though, I really appreciate you both giving me handwritten notes of how to navigate things. Think of it likes clues for the maze that help me, and hopefully future users, navigate things as well as you two :)

canyon289 · 2022-02-05T22:22:13Z

Updated per comments. Verified that its compiling locally. @OriolAbril tagging you per request, also to ensure you feel like the changes youd like have been made

OriolAbril · 2022-02-08T20:15:09Z

still have mixed feelings about the "named array" term, the caption looks good otherwise, but we should wait until fixing CI to check the preview on readthedocs.

I have one question though. Are those terms/definitions used elsewhere in the documentation? We could include those in the glossary and here so that when they appear somewhere else we can use term role and render it as a link to its glossary definition

docs/source/learn/examples/dimensionality.ipynb

canyon289 · 2022-02-13T16:02:47Z

still have mixed feelings about the "named array" term, the caption looks good otherwise, but we should wait until fixing CI to check the preview on readthedocs.

I have one question though. Are those terms/definitions used elsewhere in the documentation? We could include those in the glossary and here so that when they appear somewhere else we can use term role and render it as a link to its glossary definition

Great Idea ill add them to the glossary

canyon289 · 2022-02-13T16:07:17Z

@OriolAbril actually can we create an issue ticket to add the terms to the glossary and merge this as is? This will be a great issue for the data umbrella sprint, and worst case I'll just get back to it afterwards.

RE: Name arrays that has been changed by Michael so we should be good to go there as well

OriolAbril

@OriolAbril actually can we create an issue ticket to add the terms to the glossary and merge this as is? This will be a great issue for the data umbrella sprint, and worst case I'll just get back to it afterwards.

We can take this over in a follow up PR, and if you have time please open an issue. But I don't think it's a good issue for the sprint. It's not only a matter of copying definitions from one place to another but about making sure definitions appear in both places (pulling them from a single source of truth though) as they are key to understanding the doc, and having to click a link to read them would be a disservice.

canyon289 · 2022-02-13T21:19:08Z

Ill definitely open an issue to track.

As for this not being good for the sprint curious why would it wouldnt be? Its a straightforward change and takes folks through all the steps of compiling the docs and making a pr? Not implying you need to change your mind, just curious what I'm missing

canyon289 · 2022-02-13T21:20:05Z

Note, I'll merge this after a rebase on main to make sure Ci is passing. Will get to it in next ~72 hours

OriolAbril · 2022-02-13T21:35:11Z

The key is in the from a single source of truth syntagma. IMO both the glossary and this page should show the complete definitions, and then other pages using these terms should link to the glossary using the term role.

The first idea that comes to mind is using myst substitutions for this which would mean writing the definitions in conf.py and then having this page and the glossary. None of the webinars cover this and there are no references on how to do this anywhere in our docs, you need to know myst. Sphinx is amazing and very flexible, but I think one should first use it with a provided configuration, and once you know how to take advantage of the available features and settings and only then look at the conf.py file.

I think this is also the main reason the jupyterbook people decided to use a yaml file instead of trying to teach juypterbook users to use conf.py. I find the yaml too limiting and wouldn't use it (and you can do the same jupyterbook does and more -or less!- with conf.py directly) but I see the yaml as a very valuable resource to get people to use sphinx straight away without getting lost in conf.py which can be overwhelming and is also very tempting place for premature optimization.

OriolAbril · 2022-02-13T21:35:59Z

extra note: I don't think CI will pass until after we merge #5449

Co-authored-by: Michael Osthege <[email protected]>

canyon289 · 2022-02-13T21:42:40Z

The key is in the from a single source of truth syntagma. IMO both the glossary and this page should show the complete definitions, and then other pages using these terms should link to the glossary using the term role.

The first idea that comes to mind is using myst substitutions for this which would mean writing the definitions in conf.py and then having this page and the glossary. None of the webinars cover this and there are no references on how to do this anywhere in our docs, you need to know myst. Sphinx is amazing and very flexible, but I think one should first use it with a provided configuration, and once you know how to take advantage of the available features and settings and only then look at the conf.py file.

I think this is also the main reason the jupyterbook people decided to use a yaml file instead of trying to teach juypterbook users to use conf.py. I find the yaml too limiting and wouldn't use it (and you can do the same jupyterbook does and more -or less!- with conf.py directly) but I see the yaml as a very valuable resource to get people to use sphinx straight away without getting lost in conf.py which can be overwhelming and is also very tempting place for premature optimization.

Thanks, makes sense now.

Regarding CI if it wont pass I'll just merge this as theres zero chance broke anything in the main library

canyon289 · 2022-02-17T05:22:16Z

Merging

canyon289 commented Feb 1, 2022

View reviewed changes

docs/source/learn/examples/dimensionality.ipynb Show resolved Hide resolved

ricardoV94 reviewed Feb 1, 2022

View reviewed changes

OriolAbril requested changes Feb 1, 2022

View reviewed changes

docs/source/learn/examples.md Outdated Show resolved Hide resolved

OriolAbril reviewed Feb 1, 2022

View reviewed changes

docs/source/learn/examples/dimensionality.ipynb Show resolved Hide resolved

docs/source/learn/examples/dimensionality.ipynb Show resolved Hide resolved

docs/source/learn/examples/dimensionality.ipynb Show resolved Hide resolved

canyon289 changed the title ~~Add start of shapes doc~~ Add start of dimensionality notebook doc Feb 2, 2022

ricardoV94 reviewed Feb 2, 2022

View reviewed changes

ricardoV94 mentioned this pull request Feb 5, 2022

Refactor Multivariate distributions due to new meaning of size #5446

Closed

canyon289 mentioned this pull request Feb 5, 2022

Drop scalar parameters for multivariate dists #5447

Closed

michaelosthege reviewed Feb 8, 2022

View reviewed changes

docs/source/learn/examples/dimensionality.ipynb Outdated Show resolved Hide resolved

OriolAbril approved these changes Feb 13, 2022

View reviewed changes

canyon289 added 8 commits February 13, 2022 13:36

Add preliminary shapes doc

341108b

Update notebook

43179cf

Update dimensionality notebook

e6126bb

Rename notebook to dimensionality

8d1b61c

Address comments

561d490

Add prime number advice

b7a9f55

Fix exmaples.md name

6dec20e

Update api.rst

ba9f9fb

canyon289 and others added 3 commits February 13, 2022 13:36

Update to mu=[0,0] per comment

cc0cd22

Update shapes notebook

c627395

Update docs/source/learn/examples/dimensionality.ipynb

e4e80b7

Co-authored-by: Michael Osthege <[email protected]>

canyon289 force-pushed the shapes_docs branch from b2a691e to e4e80b7 Compare February 13, 2022 21:37

canyon289 merged commit 0f08c9e into pymc-devs:main Feb 17, 2022

canyon289 mentioned this pull request Feb 17, 2022

Add dimensonality terms to glossary #5480

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add start of dimensionality notebook doc #5437

Add start of dimensionality notebook doc #5437

canyon289 commented Feb 1, 2022 •

edited

Loading

review-notebook-app bot commented Feb 1, 2022

codecov bot commented Feb 1, 2022 •

edited

Loading

ricardoV94 Feb 1, 2022

michaelosthege Feb 1, 2022

canyon289 Feb 2, 2022

ricardoV94 Feb 2, 2022 •

edited

Loading

OriolAbril commented Feb 1, 2022

canyon289 commented Feb 2, 2022

ricardoV94 Feb 2, 2022

canyon289 Feb 4, 2022

michaelosthege Feb 4, 2022

twiecki Feb 5, 2022

ricardoV94 Feb 5, 2022 •

edited

Loading

canyon289 Feb 5, 2022 •

edited

Loading

canyon289 commented Feb 5, 2022

OriolAbril commented Feb 8, 2022 •

edited

Loading

canyon289 commented Feb 13, 2022

canyon289 commented Feb 13, 2022

OriolAbril left a comment

canyon289 commented Feb 13, 2022

canyon289 commented Feb 13, 2022 •

edited

Loading

OriolAbril commented Feb 13, 2022

OriolAbril commented Feb 13, 2022

canyon289 commented Feb 13, 2022

canyon289 commented Feb 17, 2022

Add start of dimensionality notebook doc #5437

Add start of dimensionality notebook doc #5437

Conversation

canyon289 commented Feb 1, 2022 • edited Loading

review-notebook-app bot commented Feb 1, 2022

codecov bot commented Feb 1, 2022 • edited Loading

Codecov Report

ricardoV94 Feb 1, 2022

Choose a reason for hiding this comment

michaelosthege Feb 1, 2022

Choose a reason for hiding this comment

canyon289 Feb 2, 2022

Choose a reason for hiding this comment

ricardoV94 Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

OriolAbril commented Feb 1, 2022

canyon289 commented Feb 2, 2022

ricardoV94 Feb 2, 2022

Choose a reason for hiding this comment

canyon289 Feb 4, 2022

Choose a reason for hiding this comment

michaelosthege Feb 4, 2022

Choose a reason for hiding this comment

twiecki Feb 5, 2022

Choose a reason for hiding this comment

ricardoV94 Feb 5, 2022 • edited Loading

Choose a reason for hiding this comment

canyon289 Feb 5, 2022 • edited Loading

Choose a reason for hiding this comment

canyon289 commented Feb 5, 2022

OriolAbril commented Feb 8, 2022 • edited Loading

canyon289 commented Feb 13, 2022

canyon289 commented Feb 13, 2022

OriolAbril left a comment

Choose a reason for hiding this comment

canyon289 commented Feb 13, 2022

canyon289 commented Feb 13, 2022 • edited Loading

OriolAbril commented Feb 13, 2022

OriolAbril commented Feb 13, 2022

canyon289 commented Feb 13, 2022

canyon289 commented Feb 17, 2022

canyon289 commented Feb 1, 2022 •

edited

Loading

codecov bot commented Feb 1, 2022 •

edited

Loading

ricardoV94 Feb 2, 2022 •

edited

Loading

ricardoV94 Feb 5, 2022 •

edited

Loading

canyon289 Feb 5, 2022 •

edited

Loading

OriolAbril commented Feb 8, 2022 •

edited

Loading

canyon289 commented Feb 13, 2022 •

edited

Loading