Skip to content
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

update data_container example to pymc 5.6 #559

Merged
merged 22 commits into from Jul 18, 2023

Conversation

jessegrabowski
Copy link
Member

@jessegrabowski jessegrabowski commented Jul 11, 2023

Update how_to/data_container to PyMC 5.6

Related to #333

The data container notebook is out of date. It uses PyMC3, and uses several APIs that are no longer needed/suggested/supported. Most prominent is using pm.Data instead of specifying pm.MutableData, but other small updates include:

  • Remove return_inferencedata=True from pm.sample
  • It is no longer necessary to make a custom group with az.from_pymc to add a predictions group; users should instead use the predictions=True flag in pm.sample_posterior_predictive
  • The same is also true about manually extending idata to include a posterior_predictive group; users should pass extend_idata=True instead
  • A referenced error that didn't allow setting a single observation has since been fixed

I also added a small explanation about the difference between pm.ConstantData and how to set mutable coords using model.add_coord. There might be a better way to do that last one.


📚 Documentation preview 📚: https://pymc-examples--559.org.readthedocs.build/en/559/

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@jessegrabowski
Copy link
Member Author

Just saw #527, I didn't even know this feature existed. I'll update this notebook to use it in the final example.

@OriolAbril
Copy link
Member

Thanks! This is great, we get questions about this quite often

Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

examples/howto/data_container.myst.md Outdated Show resolved Hide resolved
@jessegrabowski
Copy link
Member Author

jessegrabowski commented Jul 12, 2023

I refactored the last baby weight example to:

  1. Remove unicode greek names (following the examples style guide)
  2. Include both mutable and constant coords, to highlight the difference

I think (2) is important, since it's not really a documented feature anywhere yet.

@review-notebook-app
Copy link

review-notebook-app bot commented Jul 12, 2023

View / edit / reply to this conversation on ReviewNB

twiecki commented on 2023-07-12T09:26:25Z
----------------------------------------------------------------

Should we just rename this to Using Data containers?


jessegrabowski commented on 2023-07-12T09:46:55Z
----------------------------------------------------------------

I like that better, since users don't need to know what a "shared variable" is to use the data containers API

@review-notebook-app
Copy link

review-notebook-app bot commented Jul 12, 2023

View / edit / reply to this conversation on ReviewNB

twiecki commented on 2023-07-12T09:26:26Z
----------------------------------------------------------------

*allow you to

Maybe also mention why anyone would want to use ConstantData (performance).

Also, I think that pm.Data also allows specification of dims, no? The only reason we have Data, MutableData, and ConstantData is historical and I think we should say that best practice is to use MutableData and ConstantData and only mention Data as a side-note.


jessegrabowski commented on 2023-07-12T09:44:57Z
----------------------------------------------------------------

I was thinking it would be nice to change one of the examples to use ConstantData, so that there's one usage for both. Maybe the first example. Then we could change around some headings (to point out which container is being used in which section, and why)?

A short passage at the beginning could be added, like "In past versions of PyMC, data was added to a model using pm.Data. While this is still possible for backwards-comparability reasons, the current best practice is to use either pm.MutableData or pm.ConstantData,depending on your needs. This notebook will demonstrate how each of these data containers are used, and when they are appropriate".

twiecki commented on 2023-07-13T10:20:53Z
----------------------------------------------------------------

Yes, I think that's perfect.

jessegrabowski commented on 2023-07-13T10:59:44Z
----------------------------------------------------------------

I tried adding some details about each of the containers and give an example for each, let me know if it falls short of the mark.

@review-notebook-app
Copy link

review-notebook-app bot commented Jul 12, 2023

View / edit / reply to this conversation on ReviewNB

twiecki commented on 2023-07-12T09:26:27Z
----------------------------------------------------------------

size is internal API, either use shape, or, better yet, also a dummy-dim.


jessegrabowski commented on 2023-07-12T09:51:08Z
----------------------------------------------------------------

What do you mean by a dummy dim?

twiecki commented on 2023-07-13T10:20:20Z
----------------------------------------------------------------

Oh, I see what you're doing here now. I think it should just be shape though.

Copy link
Member Author

I was thinking it would be nice to change one of the examples to use ConstantData, so that there's one usage for both. Maybe the first example. Then we could change around some headings (to point out which container is being used in which section, and why)?

A short passage at the beginning could be added, like "In past versions of PyMC, data was added to a model using pm.Data. While this is still possible for backwards-comparability reasons, the current best practice is to use either pm.MutableData or pm.ConstantData,depending on your needs. This notebook will demonstrate how each of these data containers are used, and when they are appropriate".


View entire conversation on ReviewNB

Copy link
Member Author

I like that better, since users don't need to know what a "shared variable" is to use the data containers API


View entire conversation on ReviewNB

Copy link
Member Author

What do you mean by a dummy dim?


View entire conversation on ReviewNB

Copy link
Member

twiecki commented Jul 13, 2023

Oh, I see what you're doing here now. I think it should just be shape though.


View entire conversation on ReviewNB

Copy link
Member

twiecki commented Jul 13, 2023

Yes, I think that's perfect.


View entire conversation on ReviewNB

Copy link
Member Author

I tried adding some details about each of the containers and give an example for each, let me know if it falls short of the mark.


View entire conversation on ReviewNB

examples/howto/data_container.myst.md Outdated Show resolved Hide resolved
examples/howto/data_container.myst.md Outdated Show resolved Hide resolved
examples/howto/data_container.myst.md Outdated Show resolved Hide resolved
@@ -112,67 +116,100 @@ idata.posterior.coords
az.plot_trace(idata, var_names=["europe_mean_temp", "city_temperature"]);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
az.plot_trace(idata, var_names=["europe_mean_temp", "city_temperature"]);
az.plot_trace(idata, var_names=["europe_mean_temp", "city_temperature"], legend=True);

this should add a legend to both rows, first with chain linestyle mapping, then with city color mapping.

Copy link
Member Author

Choose a reason for hiding this comment

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

The auto-placement leaves a bit to be desired -- is there any way to pass legend_kwargs to the underlying plot?

examples/howto/data_container.myst.md Outdated Show resolved Hide resolved
examples/howto/data_container.myst.md Outdated Show resolved Hide resolved
examples/howto/data_container.myst.md Outdated Show resolved Hide resolved
examples/howto/data_container.myst.md Outdated Show resolved Hide resolved
Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

added one extra comment, also forgot to mention that I review on the myst file, but you should work on the ipynb and let pre-commit update the myst one

examples/howto/data_container.myst.md Show resolved Hide resolved
examples/howto/data_container.myst.md Outdated Show resolved Hide resolved
examples/howto/data_container.myst.md Outdated Show resolved Hide resolved
Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

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

Looks good, I left a couple of smaller suggestions

examples/howto/data_container.myst.md Outdated Show resolved Hide resolved
examples/howto/data_container.myst.md Show resolved Hide resolved
examples/howto/data_container.myst.md Outdated Show resolved Hide resolved
examples/howto/data_container.myst.md Outdated Show resolved Hide resolved
examples/howto/data_container.myst.md Outdated Show resolved Hide resolved
examples/howto/data_container.myst.md Outdated Show resolved Hide resolved
examples/howto/data_container.myst.md Outdated Show resolved Hide resolved
Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

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

Shaping up really nice, just some minor suggestions

examples/howto/data_container.myst.md Outdated Show resolved Hide resolved
examples/howto/data_container.myst.md Outdated Show resolved Hide resolved
examples/howto/data_container.myst.md Outdated Show resolved Hide resolved
Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

noticed one typo (again, fix in ipynb, then pre-commit), but other than that I think it is good to merge

examples/howto/data_container.myst.md Outdated Show resolved Hide resolved
examples/howto/data_container.myst.md Outdated Show resolved Hide resolved
examples/howto/data_container.myst.md Outdated Show resolved Hide resolved
@ricardoV94 ricardoV94 merged commit d6df689 into pymc-devs:main Jul 18, 2023
2 checks passed
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.

None yet

4 participants