Skip to content

Example for FuncAnimation, passing args to animation guide #27675

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vamshipy
Copy link

Added a file to show how args are passed to FuncAnimation to create a graph

PR summary

Fixes #27621

PR checklist

@github-actions github-actions bot added the Documentation: examples files in galleries/examples label Jan 19, 2024
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

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.

Hi, thanks for this PR! We already have an example of passing arguments https://matplotlib.org/devdocs/gallery/animation/animated_histogram.html#sphx-glr-gallery-animation-animated-histogram-py but I think that example may be more of a small tutorial than an example; therefore I'm not opposed to adding this, especially since it's also an example of animating text, once changes are made.

To help you resolve the flake errors, I strongly recommend that you install the precommit hooks:
https://matplotlib.org/devdocs/devel/development_setup.html#install-pre-commit-hooks

That being said, my issue request was for adding a second small example to https://matplotlib.org/devdocs/users/explain/animations/animations.html#funcanimation showing a parameterized update function.

@vamshipy
Copy link
Author

Hi, thanks for this PR! We already have an example of passing arguments https://matplotlib.org/devdocs/gallery/animation/animated_histogram.html#sphx-glr-gallery-animation-animated-histogram-py but I think that example may be more of a small tutorial than an example; therefore I'm not opposed to adding this, especially since it's also an example of animating text, once changes are made.

To help you resolve the flake errors, I strongly recommend that you install the precommit hooks: https://matplotlib.org/devdocs/devel/development_setup.html#install-pre-commit-hooks

That being said, my issue request was for adding a second small example to https://matplotlib.org/devdocs/users/explain/animations/animations.html#funcanimation showing a parameterized update function.

Thank you for feedback, so the issue was to just create an example/ tutorial for the update function my mistake..

@vamshipy vamshipy requested a review from story645 January 21, 2024 03:55
@story645
Copy link
Member

so the issue was to just create an example/ tutorial for the update function my mistake..

It was to add a "passing arguments" subsection to the FuncAnimate section of the Animation User Guide:

https://github.com/matplotlib/matplotlib/blob/main/galleries/users_explain/animations/animations.py#L127

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.

Tiny formatting convention but otherwise this is looking really good! Thanks 😄

To clean up the PR cleanliness error, I think you'll have to rebase https://matplotlib.org/devdocs/devel/development_workflow.html#rewrite-commit-history

Let me know if you need help w/ that.

@story645 story645 added this to the v3.8-doc milestone Jan 23, 2024
@timhoffm
Copy link
Member

I'm sorry, but what is the purpose of the example? As far as I understood #27621 was requesting additions to https://matplotlib.org/devdocs/users/explain/animations/animations.html#animations.

@story645
Copy link
Member

As far as I understood #27621 was requesting additions to https://matplotlib.org/devdocs/users/explain/animations/animations.html#animations.

Yeah, but honestly that issue is superceded by #27687 anyway

Reason I was letting this PR go through anyway though was b/c we don't have an explicit "how to pass arguments to animation function" example (one's kinda buried in the histogram example that's more a tutorial) and this one felt pretty clean and straightforward.

@vamshipy
Copy link
Author

Tiny formatting convention but otherwise this is looking really good! Thanks 😄

To clean up the PR cleanliness error, I think you'll have to rebase https://matplotlib.org/devdocs/devel/development_workflow.html#rewrite-commit-history

Let me know if you need help w/ that.

thank you for your help, the workflows seem very intimidating definitely need help with rebasing

@story645
Copy link
Member

story645 commented Jan 24, 2024

@vamshipy just pushed a rebase and also made some edits to make the example more concise. I'm gonna leave this PR for someone else to review b/c I don't feel comfortable merging on just my approval given the extent of the changes I've made to this PR.

@story645 story645 dismissed their stale review January 24, 2024 03:45

made rest of changes while rebasing PR

@vamshipy
Copy link
Author

@vamshipy just pushed a rebase and also made some edits to make the example more concise. I'm gonna leave this PR for someone else to review b/c I don't feel comfortable merging on just my approval given the extent of the changes I've made to this PR.

I understand, is there something I have to do about the labeler error

@story645
Copy link
Member

I understand, is there something I have to do about the labeler error

No, that's a bug on our end.

Do you have any comments/changes you'd like to suggest on top of my changes?

Co-authored-by: hannah <story645@gmail.com>
@timhoffm
Copy link
Member

Reason I was letting this PR go through anyway though was b/c we don't have an explicit "how to pass arguments to animation function" example.

I'm unclear when why one should pass arguments to the animation function. Other examples achieve the same as here by reusing variables from the outer scope.

While knowing and relying on the scoping is possibly not trivial, it just works for the usual cases. It's not necessarily conceptually simpler/better to rely on a partial closure. If we want to add this, I'd like to see an explanation why one would want to use partial and how it works.

and this one felt pretty clean and straightforward.

With four parameters passed to partial, this is more complex than need be. One (or two if you want data and an artist) should be enough. Reducing to a minimal example helps in understanding the concept and not get lost in details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Waiting for author
Development

Successfully merging this pull request may close these issues.

Doc: Add passing args to Animation guide
3 participants