Skip to content

Feature Proposal: SVGFuncAnimation #20142

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Feature Proposal: SVGFuncAnimation #20142

wants to merge 7 commits into from

Conversation

jungerm2
Copy link

@jungerm2 jungerm2 commented May 3, 2021 •
edited
Loading

PR Summary

This PR stems from #19694. It implements a new animation class SVGFuncAnimation that exclusively creates SVG animations. Because of this restriction, it can reuse components that are common between frames and thus only minimally updates the SVG dom for each frame. In my tests, I found that this results in a speedup of up to 30x and about half the memory footprint as compared to the standard FuncAnimation.

It's meant to be (almost) a drop-in for FuncAnimation and can be used as such:

np.random.seed(0)
size = 10

def update_line(num, data, line):
    line.set_data(range(num), data[:num])
    return line,

fig = plt.figure()
data = np.random.rand(size)
l, = plt.plot([], [], 'r-')
plt.xlim(0, size - 1)
plt.ylim(0, 1)

anim = SVGFuncAnimation(fig, update_line, range(1, size + 1), fargs=(data, l))

I have a few other examples here and a basic benchmark here.

This is a new feature (and my first PR here) so I suspect there'll be some discussion about what the proper API for this is and quite a few things to correct/fix. This is part of the reason I have yet to write more extensive docs. Any and all feedback is welcomed!

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@jklymak
Copy link
Member

jklymak commented May 3, 2021

Perhaps give us a mock example in the PR description?

github-actions[bot]
github-actions bot reviewed May 3, 2021
View reviewed changes
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 while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. 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.

@jungerm2
Copy link
Author

jungerm2 commented May 3, 2021 •
edited
Loading

It looks like the signature of functool's lru_cache changed after py37. Should be a quick fix, I'll push an update to this shortly.

EDIT: It's fixed now. All tests pass 👍

@QuLogic
Copy link
Member

QuLogic commented May 4, 2021

What makes this specifically need to be an Animation, and not a Writer?

@jungerm2
Copy link
Author

jungerm2 commented May 4, 2021

Originally, instead of hooking into the SVG backend to intercept the frame data, I had it parse every frame separately which meant it couldn't be just a Writer. The current code is more simple so we might be able to make it inherit from MovieWriter or FileMovieWriter.

One of the subtleties with making it a Writer would be whether or not to extend the HTMLWriter or simply create its own class. After all the output format of the SVGFunAnimation is pretty similar to the output of the HTMLWriter. It uses a similar jshtml output except that the js edits the SVG dom instead of swapping out an img tag. We'd need a separate to_svghtml or maybe just add an option to the to_jshtml call if we make this a writer.

@jklymak jklymak requested a review from tacaswell May 15, 2021 15:38
@jklymak
Copy link
Member

jklymak commented May 15, 2021

@tacaswell, you encouraged this to be PRd, so I'll ping you to shepherd this through if you have the bandwidth? @jungerm2 did a lot of work here, and it seems reasonable to me, but its a lot of new code...

@tacaswell
Copy link
Member

tacaswell commented May 15, 2021 •
edited
Loading

I will not realistically have bandwidth to look at this until next week.

@jungerm2 If you do not hear from me by May 26 please ping me!

@jungerm2
Copy link
Author

Will do, Thanks! Let me know if there's anything I can help with too (time permitting). But as @QuLogic mentioned it might make more sense to make this into a Writer, I'm just not totally sure how that would work...

@jklymak jklymak marked this pull request as draft May 22, 2021 15:50
@jungerm2
Copy link
Author

jungerm2 commented Jun 1, 2021

@tacaswell were you able to take a look at this PR yet?

@melissawm
Copy link
Member

Hi @jungerm2 - I'm sorry this fell through the cracks, are you interested in continuing this work? If so, please mark the PR as ready for review and let us know if you need help with the rebase, we have instructions for that here. Thanks!

@jungerm2
Copy link
Author

Hey, thanks for getting back to me! It's been a while since I've worked on this but still think it would add value to matplotlib, but it seems there are quite a few merge conflicts now. And as I've said this might be better implemented as a Writer class. It would be great to get some feedback on this.

If there's sufficient interest I might pick this back up but don't have the bandwidth to merge this in at the moment.

@tacaswell
Copy link
Member

I took the liberty of doing the rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@tacaswell tacaswell Awaiting requested review from tacaswell

At least 1 approving review is required to merge this pull request.

Assignees

@tacaswell tacaswell

Labels
backend: svg New feature status: needs comment/discussion needs consensus on next step status: needs rebase topic: animation
Projects
First Time Contributors
Status: Needs decision
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jungerm2 @jklymak @QuLogic @tacaswell @melissawm