Replacing PyMC3 plots w/ Arviz plots & sigma Param change [Part 6]

[CloudChaoszero]

Description

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

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

• The issue is a 3-part step for the following:
  • Initiative via Issue Remove Plots & Diagnostics from PyMC Code Base pymc#4371 (Leave open)

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

• Closes issue Use sigma instead of sd #2

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

twiecki commented on 2021-03-08T11:03:10Z
----------------------------------------------------------------

Need to set up a single ax object and pass that to every plot() call.

OriolAbril commented on 2021-03-08T16:11:33Z
----------------------------------------------------------------

if you start with the hdi plot, ArviZ will generate the axes and return it.

CloudChaoszero commented on 2021-03-14T04:28:26Z
----------------------------------------------------------------

Sounds great, done! Thanks for the tips

[twiecki]

best to remove.

Reply via ReviewNB

[twiecki]

@CloudChaoszero Sorry this took so long to get to. Are you still up for pushing this over the finish line? I think the last outstanding issues are pretty small and we should be able to resolve them quickly.

[OriolAbril]

I can checkout a couple of the PRs locally to finish with the last nits and push back here for merging if you have checked this box:

[OriolAbril]

if you start with the hdi plot, ArviZ will generate the axes and return it. Not sure if this is a practice to be recommended, but maybe?

---

View entire conversation on ReviewNB

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-03-08T16:28:58Z
----------------------------------------------------------------

I would remove the samples argument. Sampling takes 1 second so there isn't any reason to thin the samples

CloudChaoszero commented on 2021-03-14T04:28:35Z
----------------------------------------------------------------

Sounds good!

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-03-08T16:28:59Z
----------------------------------------------------------------

I would use keep_size=True to avoid the warning in the cell below, also because in a future ArviZ release the behaviour of hdi will change for 2d arrays (not for 1d or 3d+ arrays), so using 3d arrays with chain, draw, *shape should be used.

CloudChaoszero commented on 2021-03-14T04:29:04Z
----------------------------------------------------------------

I re-ran the visualizations, and they did not have an outputted warning cell (except for subsequent visualizations). Thanks!

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-03-08T16:29:00Z
----------------------------------------------------------------

I would remove the 2000 here too, and let the default of as many samples as the posterior be used.

[OriolAbril]

return_inferencedata should be manually set, either to True or to False, but one of the two to avoid the warning.

If False, an inferencedata should be created within the model context, and passed to arviz. This will 1) avoid the warning of conversion without the model context and 2) push forward arviz best practices, it is probably not too relevant here but conversion may not be cheap for some models because it requires computing all the poitnwise log likelihood values. az.plot_xyz(trace) works because ArviZ internally converts the data to inferencedata, then plots.

Reply via ReviewNB

[OriolAbril]

I would use az.plot_pair here, it can also use hexbin mode, some examples: https://arviz-devs.github.io/arviz/examples/plot_pair_hex.html and https://arviz-devs.github.io/arviz/examples/plot_joint.html. Side advantages, I think with that we can use return_inferencedata=True directly and the plot will be automatically labeled

Reply via ReviewNB

[OriolAbril]

as we are not storing the summary dataframe anywhere and we only want the rhat, we should use rhat instead. The assertion can be done with:

assert (az.rhat(trace).to_array() < 1.1).all()

Reply via ReviewNB

[CloudChaoszero]

Unsure. Just re-ran the pre-existing notebooks. haha

[CloudChaoszero]

Sounds great, done! Thanks for the tips

---

View entire conversation on ReviewNB

[CloudChaoszero]

Sounds good!

---

View entire conversation on ReviewNB

[CloudChaoszero]

I re-ran the visualizations, and they did not have an outputted warning cell (except for subsequent visualizations). Thanks!

---

View entire conversation on ReviewNB

[CloudChaoszero]

Pardon for the delay @twiecki. I made the respective updates.

Thanks for the feedback @OriolAbril ! It really helped out on a couple of outputs. I did not act on a couple of other ones because they would probably be for future development (e.g. returninference or some additional viz customizations).

Also, I disabled the specified button for this PR, seen below

[twiecki]

Thanks @CloudChaoszero!

[OriolAbril]

I did not act on a couple of other ones because they would probably be for future development (e.g. returninference or some additional viz customizations).

@CloudChaoszero can you please create issues about all the comments you have not addressed so other people can easily know low hanging fruit tasks on which to work regarding notebooks?

[CloudChaoszero]

I did not act on a couple of other ones because they would probably be for future development (e.g. returninference or some additional viz customizations).

@CloudChaoszero can you please create issues about all the comments you have not addressed so other people can easily know low hanging fruit tasks on which to work regarding notebooks?

Sounds good!