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

plot_ppc animation: improve docs and error handling #1162

Merged
merged 7 commits into from
Apr 30, 2020
Merged

Conversation

aloctavodia
Copy link
Contributor

@aloctavodia aloctavodia commented Apr 21, 2020

Following #1153 this PR adds a more detailed explanation of common problems and fixes arising when running animations. It will also raise a Warning when tryuing to render and animation inside a notebook without the

  • Follows official PR format
  • Code style correct (follows pylint and black guidelines)
  • Changes are listed in changelog

"`animation_kwargs({'blit':False}) or changing the plotting backend (e.g. to TkAgg)"
)
if animated:
if backend == "matplotlib":
Copy link
Member

@canyon289 canyon289 Apr 26, 2020

Choose a reason for hiding this comment

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

This logic should be moved into the backends themselves, so the higher level plotting function can stay unopinionated .

Let me know if you want me to edit PR. Happy to do so if you like!

if animated and backend == "bokeh":
raise TypeError("Animation option is only supported with matplotlib backend.")

if animated and animation_kwargs["blit"] and platform.system() != "Linux":
Copy link
Member

Choose a reason for hiding this comment

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

Noticed this was already here before your PR. Tech debt I guess!

@@ -312,10 +339,6 @@ def plot_ppc(
show=show,
)

if backend is None:
Copy link
Member

@canyon289 canyon289 Apr 26, 2020

Choose a reason for hiding this comment

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

Thanks for moving this

@canyon289
Copy link
Member

I should note happy to get this merged to fix the animation issue and I can follow up to move the code to the appropriate backend.

@codecov
Copy link

codecov bot commented Apr 30, 2020

Codecov Report

Merging #1162 into master will increase coverage by 14.32%.
The diff coverage is 83.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1162       +/-   ##
===========================================
+ Coverage   78.77%   93.10%   +14.32%     
===========================================
  Files          94       94               
  Lines        9302     9312       +10     
===========================================
+ Hits         7328     8670     +1342     
+ Misses       1974      642     -1332     
Impacted Files Coverage Δ
arviz/plots/ppcplot.py 95.18% <80.00%> (-0.12%) ⬇️
arviz/plots/backends/matplotlib/ppcplot.py 98.42% <84.61%> (-1.02%) ⬇️
arviz/utils.py 91.49% <0.00%> (+0.40%) ⬆️
arviz/data/inference_data.py 81.88% <0.00%> (+1.39%) ⬆️
arviz/data/base.py 100.00% <0.00%> (+10.97%) ⬆️
arviz/data/io_cmdstan.py 94.84% <0.00%> (+51.07%) ⬆️
arviz/data/io_pymc3.py 92.64% <0.00%> (+69.69%) ⬆️
arviz/data/io_pyro.py 95.86% <0.00%> (+71.03%) ⬆️
arviz/data/io_numpyro.py 95.23% <0.00%> (+71.42%) ⬆️
arviz/data/io_cmdstanpy.py 100.00% <0.00%> (+78.30%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebd5231...fbf05f5. Read the comment docs.

@canyon289
Copy link
Member

LGTM. Merging

@canyon289 canyon289 merged commit 7ab50b0 into master Apr 30, 2020
@OriolAbril OriolAbril deleted the anim_warning branch April 30, 2020 16:42
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