Skip to content

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