Skip to content

DOC: simplify histogram animation example #27620

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

Merged
merged 2 commits into from
Jan 9, 2024

Conversation

rcomer
Copy link
Member

@rcomer rcomer commented Jan 9, 2024

PR summary

I might be missing something but the function-returning-a-function approach used here seems unnecessarily complicated to me. We can just have our animate function update the global BarContainer object.

While I was in there I also

  • Updated to use numpy's default_rng, per #25765(comment)
  • Made punctuation more consistent through the comments

PR checklist

@rcomer rcomer added the Documentation: examples files in galleries/examples label Jan 9, 2024
@rcomer rcomer added this to the v3.8-doc milestone Jan 9, 2024
story645
story645 previously approved these changes Jan 9, 2024
Copy link
Member

@story645 story645 left a comment

Choose a reason for hiding this comment

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

😄

Can't decide if helpful or out of scope to also show updating a histogram using ArtistAnimation

@dstansby
Copy link
Member

dstansby commented Jan 9, 2024

This can be merged if/when CI passes.

@rcomer
Copy link
Member Author

rcomer commented Jan 9, 2024

Renders like this

The animation is no longer showing in the built docs, just a static image. Any ideas how I broke that?

@rcomer rcomer marked this pull request as draft January 9, 2024 16:17
@story645
Copy link
Member

story645 commented Jan 9, 2024

The animation is no longer showing in the built docs, just a static image. Any ideas how I broke that?

So my guess is that the reason for the wrapper function is b/c otherwise sphinx gallery is losing the figure/global that's in the other cell. 🤦‍♀️

yeah, see how in the current the figure is created in the same block as the call to the animation function https://matplotlib.org/devdocs/gallery/animation/animated_histogram.html#sphx-glr-gallery-animation-animated-histogram-py

@story645 story645 dismissed their stale review January 9, 2024 16:30

oops, guess wrapper was important

@rcomer
Copy link
Member Author

rcomer commented Jan 9, 2024

Thanks @story645 I have moved the figure and artists setup back to the last cell and the animation works again. I think it is a bit less intuitive this way round, but still good to remove the unnecessary complication of the wrapper function.

@rcomer rcomer marked this pull request as ready for review January 9, 2024 17:14
@story645
Copy link
Member

story645 commented Jan 9, 2024

but still good to remove the unnecessary complication of the wrapper function.

So I just realized this could maybe be a good place to demo how we can pass arguments to animation update functions using partial:
https://matplotlib.org/devdocs/api/_as_gen/matplotlib.animation.FuncAnimation.html#matplotlib.animation.FuncAnimation

But also not a blocker, current globals is fine too/ I can do as follow up.

@rcomer
Copy link
Member Author

rcomer commented Jan 9, 2024

demo how we can pass arguments to animation update functions using partial

I like this. It feels more intuitive given the order of cells in the example. Also we already have the line animation to demonstrate the absolute simplest way of doing it.

@story645 story645 merged commit 83aa3e4 into matplotlib:main Jan 9, 2024
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Jan 9, 2024
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Jan 9, 2024
QuLogic added a commit that referenced this pull request Jan 9, 2024
…620-on-v3.8.x

Backport PR #27620 on branch v3.8.x (DOC: simplify histogram animation example)
QuLogic added a commit that referenced this pull request Jan 9, 2024
…620-on-v3.8.2-doc

Backport PR #27620 on branch v3.8.2-doc (DOC: simplify histogram animation example)
@rcomer rcomer deleted the doc-hist-anim branch January 10, 2024 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation: examples files in galleries/examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants