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

Samplerstats(WIP) #94

Closed
wants to merge 0 commits into from
Closed

Samplerstats(WIP) #94

wants to merge 0 commits into from

Conversation

mjhajharia
Copy link
Member

This notebook uses InferenceData and ArviZ for plotting.
Changed sampler_stats description according to - https://arviz-devs.github.io/arviz/schema/schema.html#sample-stats

Addresses issue #46

Still working on:

  • parallel plot idea?
  • pedagogical info about sampler stats
  • possibly expanding and adding a few more plots about sampler stats(try and see if more arviz plots can be incorporated - like dist_plot etc)

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@mjhajharia
Copy link
Member Author

@OriolAbril I was wondering about that parallel plot thing for divergences, how should we go about it, in this case i didn't get any divergences so I didn't create that..

examples/diagnostics_and_criticism/sampler-stats.ipynb Outdated Show resolved Hide resolved
examples/diagnostics_and_criticism/sampler-stats.ipynb Outdated Show resolved Hide resolved
examples/diagnostics_and_criticism/sampler-stats.ipynb Outdated Show resolved Hide resolved
@@ -14,58 +14,56 @@
},
Copy link
Member

Choose a reason for hiding this comment

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

This plot looks useless to me, I have asked around to see if anyone can help me. Will comment on that once I know what to say 😅


Reply via ReviewNB

@@ -14,58 +14,56 @@
},
Copy link
Member

Choose a reason for hiding this comment

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

I think this plot should not be a line plot but a dot plot, try using

trace.sample_stats["tree_depth"].plot(row="chain", ls="none", marker=".", alpha=.3)

which should replace the whole cell. Also, style comment only, using hue="chain" might look better


Reply via ReviewNB

examples/diagnostics_and_criticism/sampler-stats.ipynb Outdated Show resolved Hide resolved
@@ -14,58 +14,56 @@
},
Copy link
Member

Choose a reason for hiding this comment

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

no need for plt.show


Reply via ReviewNB

@@ -14,58 +14,56 @@
},
Copy link
Member

Choose a reason for hiding this comment

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

I added an issue about this, but I think we can do that manually in the meantime, also to show one can customize the InferenceData with idata_kwargs. In this particular case, I'd use:

coords = {"step": ["BinaryMetropolis", "Metropolis"]}
dims = {"accept": ["step"]}
...
   trace = pm.sample(..., idata_kwargs={"dims": dims, "coords": coords})

Reply via ReviewNB

@@ -14,58 +14,56 @@
},
Copy link
Member

Choose a reason for hiding this comment

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

Here use .data_vars


Reply via ReviewNB

@@ -14,58 +14,56 @@
},
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 not very informative, at least to me, so I'd use plot_posterior like we used above. It should automatically create two plots, one per step function an as we are customizing the dims and coords, it will be correctly labeled automatically


Reply via ReviewNB

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.

It looks like you created this branch from your other PR instead of doing so from main, I can see all the changes here in this PR. You can do for example

git rebase -i HEAD~15

to open a file with the last 15 commits, there you can pick and choose which to keep. You should be able to delete the ones about the rugby notebook (delete them from this branch, this won't affect the rugby branch). and keep the original ones (from main) and the ones for the sample stats notebook.

Rebasing can be confising at first, so if you prefer, feel free to copy the notebook you have now here, create a new branch from main, paste the notebook there and open a new PR. Both solutions will work, there is no shame in opening a new PR. But this or the new PR should not modify the rugby file to avoid merge conflicts.

parallel plot idea?

Let's just link to the divergences notebook, I'm writing this after the review, so now I'll kind of contradict myself, but here is what I'd do. Use the code in the comment to see if there are divergences, and add a note in markdown saying there are none, and that if there were, the divergences notebook will guide through diagnosing them.

@mjhajharia mjhajharia closed this Apr 2, 2021
@mjhajharia mjhajharia deleted the samplerstats branch April 2, 2021 18:13
@mjhajharia mjhajharia mentioned this pull request Apr 2, 2021
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.

2 participants