add cache_frame_data kwarg into FuncAnimation. fixes #8528.
#11894
Conversation
05bd0c3 to
6254645
Compare
|
But Is this solving a problem you've run into? The discussion in #8528 indicates that removing this did not solve the issue for the user there. |
|
Thanks for your feedback, @dopplershift My understanding is that Below is a code snippet which exhibits a memory leak at about 1MB/sec before this fix. Another way to interpret this bug is that Why does |
|
If you call save on an animation that was consuming from a generator, add |
There was a problem hiding this comment.
As implemented this is removing the ability to save an animation feed from a generator with the same data you just saw.
Currently the last save_count frames will be saved. This implementation will save the frist save_count of re-calling the generator function. If you passed in a generator object, and let it be exhausted I think this implementation will save no frames.
I do like the test, but it should check that all but save_count have been gc'd.
|
If you change your generator to run through 10k iterations and change We probably should change the logic on There probably should be a flag to disable caching. It is the right thing in many but not all cases. |
|
Thanks for your feedkback, @tacaswell So, if I understood your comment correctly, Animation is required to support saving its content even after it has already been drawn/shown interactively. Assuming it's required to support save() even after drawing is complete, I'm afraid to say that this contract forces the implementation of Animation expensive because it needs to keep all frame data. Having said that, I like your last comment on having a flag to disable caching in FuncAnimation. |
This is the current behavior (and has been there from when the code came in in 2010 70e401f) so we can not change it with out a deprecation cycle. This is one of the challenges of working on these old, big projects, it is easy to pick any part of the API and say "It would be better if...." but someone is the past thought the current API was a good one and it is likely that there are users depending on that behavior that we can not break without warning (and a good reason). https://xkcd.com/1172/ is real!
This only gets expensive under some conditions (consuming from a generator that generates and yields objects with a memory footpirnt). I have personally used this feature when working interactively (make Maybe Could this problem also be solved by passing |
6254645 to
017d88b
Compare
cache_frame_data kwarg into FuncAnimation. fixes #8528.
|
Thanks for your detailed/helpful feedback, @tacaswell |
48c3d9a to
ad7103a
Compare
ad7103a to
f8b14c8
Compare
|
Coverage is complaining about https://codecov.io/gh/matplotlib/matplotlib/compare/e7719da6cad1d34c7a199833a822f4d2ec076ac8...f8b14c8fc057ef4ed0a67c85df007f5062a4879d/src/lib/matplotlib/tests/test_animation.py?before=lib/matplotlib/tests/test_animation.py#L289 which is a bit odd. Otherwise 👍 |
There was a problem hiding this comment.
Modulo a whats_new entry in https://github.com/matplotlib/matplotlib/tree/master/doc/users/next_whats_new and maybe that extra assertion in the test.
|
Thanks for following up on this @pclove1 ! |
|
@tacaswell Thanks for your helpful reviews. |
3d4f958 to
1f908e6
Compare
|
Squashed commits and rebased against the latest master head |
|
Travis failure is #13137 and unrelated to the PR. |
|
Thanks, and congratulations on your first contribution to matplotlib. Hope to see you back some time. |
|
Like wise. |
PR Summary
cache_frame_datakwarg intoFuncAnimation.PR Checklist