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.

Sign up for GitHub

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

Jump to bottom

Replacing PyMC3 plots w/ Arviz plots & sigma Param change [Part 9] #21

Merged
merged 3 commits into from
Mar 14, 2021
Merged

Replacing PyMC3 plots w/ Arviz plots & sigma Param change [Part 9] #21

merged 3 commits into from
Mar 14, 2021

Conversation

CloudChaoszero
Copy link
Contributor

Description

The following is a large PR breakdown of PR #16.

Replace PyMC3 dependent plots with arviz plots in case studies & examples.

Replace parameter sd with sigma (e.g. some examples have pm.Normal(...sd=...)

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

twiecki
twiecki reviewed Mar 8, 2021
View reviewed changes
examples/variational_inference/empirical-approx-overview.ipynb Outdated Show resolved Hide resolved
examples/variational_inference/empirical-approx-overview.ipynb Outdated
@@ -18,11 +18,13 @@
"name": "stdout",
Copy link
Member

Choose a reason for hiding this comment

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

This looks odd. I think more recent arviz would plot them better.


Reply via ReviewNB

Copy link
Member

Choose a reason for hiding this comment

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

This is probably right, we improved the kde a couple releases back, the original implementation was too smoothing and did quite a bad job for multimodal and mixture distributions. The PR with the changes has some comparisons: https://github.com/arviz-devs/arviz/pull/1284

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds great, thanks for the reference. Updated arviz dependency now combines the two into 1 graph.

examples/variational_inference/empirical-approx-overview.ipynb Outdated
@@ -18,11 +18,13 @@
"name": "stdout",
Copy link
Member

Choose a reason for hiding this comment

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

Missing output.


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.

Done!

Note:

There was an update for the sns.kdeplot plot functionality, and created an error.

I went ahead and provided the same visualization by creating the 100x2 array into a DataFrame, and specified index in kdeplot params.

@CloudChaoszero CloudChaoszero requested a review from twiecki March 14, 2021 03:33
@twiecki twiecki merged commit 48af2ff into pymc-devs:main Mar 14, 2021
@CloudChaoszero CloudChaoszero deleted the replace-pymc3-arviz-plots_part9 branch April 5, 2021 01:53
twiecki pushed a commit that referenced this pull request Jan 17, 2023
@aloctavodia
Merge pull request #21 from pymc-devs/rephrase-readme
9db597a 
Rephrase to reflect maturation of the project
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OriolAbril OriolAbril OriolAbril left review comments

@twiecki twiecki Awaiting requested review from twiecki

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@CloudChaoszero @twiecki @OriolAbril