Deprecate attributes and expire deprecation in animation #24131

oscargus · 2022-10-08T17:07:14Z

PR Summary

Not sure if the deprecation warning could actually be triggered without setting anim.save_count after creating the animation. Anyway, it's been around from 2.2 so I think this is OK.

The Python bug mentioned seems to have been fixed in 3.8(?): python/cpython#74722

Privatized the save_count attribute as well as the repeat attribute. I cannot really see a use case for changing these after creating the animation.

PR Checklist

Tests and Styling

[N/A] Has pytest style unit tests (and pytest passes).
Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

[N/A] New features are documented, with examples if plot related.
[N/A] 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).
Documentation is sphinx and numpydoc compliant (the docs should build without error).

oscargus · 2022-10-10T07:48:08Z

I do not understand the warnings:

/home/circleci/project/doc/api/_as_gen/matplotlib.animation.ArtistAnimation.rst:33:<autosummary>:1: WARNING: py:obj reference target not found: matplotlib.animation.ArtistAnimation.repeat
/home/circleci/project/lib/matplotlib/animation.py:docstring of matplotlib.animation.FuncAnimation.new_frame_seq:1:<autosummary>:1: WARNING: py:obj reference target not found: matplotlib.animation.FuncAnimation.repeat

These are "new" attributes, in the sense that they are defined in the class rather than in __init__, but only for the purpose of being deprecated. And the save_count attribute does not behave like that. As far as I can tell, none in referenced explicitly, so should be some sort of autodoc thing.

(I know that I've had similar issues with attributes earlier though...)

QuLogic · 2022-10-13T08:44:28Z

I don't think save_count=None was the intended removal, the default of 100 was; see #8278.

oscargus · 2022-10-13T15:56:47Z

Yeah, you are right. Forgot about the purpose a bit in...

oscargus · 2022-10-15T11:28:07Z

It seems like other parts of the code are relying on save_count being an integer (either provided or computed). So the behaviour discussed in #8278 is not straightforward to obain (which may be the reason it not has been removed since 2.2...).

I guess that one can protect the line

self._save_seq = self._save_seq[-self._save_count:]

with a None-check though...

story645 · 2022-10-30T13:37:36Z

Um what happens in the infinite case if there's no trim?

QuLogic · 2022-11-01T05:52:07Z

Then I think @anntzer has argued that it keeps running unless you specify save_count, just as it does with infinite generators elsewhere.

story645 · 2022-11-01T15:40:32Z

Then I think @anntzer has argued that it keeps running unless you specify save_count, just as it does with infinite generators elsewhere.

how do you save an infinitely running movie?

oscargus · 2022-11-07T09:42:52Z

how do you save an infinitely running movie?

I guess that there is a file size limit:

Animation size has reached 21248815 bytes, exceeding the limit of 20971520.0. If you're sure you want a larger animation embedded, set the animation.embed_limit rc parameter to a larger value (in MB). This and further frames will be dropped.

tacaswell · 2022-12-15T18:50:56Z

@story645 is correct that there is an unbounded cache in the case of an inifinte generator that needs to be addressed. The cap on the file size is also only in the jshtml export logic (so we do not make a multi-gigabyte html page 🤣, related if you want to see your browser break build our docs as singlehtml, but I digress).

I pushed some commits that protect against this. There might need to be some more thought around the default value of cache_data_frames as well. I think this is going to hard break some use-cases, but those cases should also have been getting user visible warnings from 2.2 (which is 7 mpl versions).

Also working on the rebase and will add some tests.

tacaswell · 2022-12-15T18:52:56Z

and we are leaving the footcannon of trying to save an infinite generator, we just won't cache the data along the way. I think we have to leave this as it is unlikely we will be able to solve the halting problem.

This prevents a possible run-away unbounded cache!

tacaswell

But I pushed a bunch of code to this PR so this should count as half.

doc/api/next_api_changes/behavior/24131-OG.rst

lib/matplotlib/tests/test_animation.py

.circleci/config.yml

oscargus · 2022-12-17T12:23:00Z

I've tried to squash locally, but it just gets messed up. Please squash on merge...

oscargus · 2022-12-17T12:27:22Z

Feels like I've lost track of this, but I think that everything is considered now. The cleanliness may not be correct (in that case it is added yet again, since it is there).

So please squash upon merge... :-)

lib/matplotlib/animation.py

greglucas · 2022-12-17T17:33:16Z

I've tried to squash locally, but it just gets messed up. Please squash on merge...

You should be able to reset to a point you want to be at, including your remote, and then start cleaning up.

git reset --hard origin/animationdeprecations
git rebase -i upstream/main
# clean up
git push --force-with-lease

doc/api/next_api_changes/behavior/24131-OG.rst

QuLogic · 2022-12-21T21:50:16Z

examples/animation/animate_decay.py

+ani = animation.FuncAnimation(fig, run, data_gen, interval=100, init_func=init,
+                              save_count=100)


Is this needed for sphinx-gallery? There isn't otherwise any saving going on in any of these examples.

yeah, SG saves both a gif thumbnail and a js_html output to embed in the docs.

tacaswell · 2022-12-21T22:09:32Z

I'm working on sorting out what is going on with the history here.

Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>

tacaswell · 2022-12-21T22:15:58Z

I think what happened is I had rebased and force-pushed, then Oscar merged that back into his branch and then added some more changes on top.

I started from just before the merge and then cherry-picked the exrta commits on top and force-pushed again.

oscargus · 2022-12-27T09:59:24Z

Yes, there was a bit of a hassle with commits, especially since I've been using two different computers. Have to learn how to deal with that in a better way...

(The main "issue" is that there will be a lot of intermediate conflicts while rebasing although the final diff will not have any conflicts...)

Just need to sort out the doc failure then. (And possibly use privatize_attribute? I seem to recall that it was used at some stage...)

oscargus · 2022-12-27T10:34:26Z

Tried to fix the doc-failure by updating missing references. Seems to make sense since this became a problem for a deprecated attribute that will go away soon anyway. (And it removes a few hundred lines from this file.)

tacaswell · 2023-01-05T00:58:26Z

(The main "issue" is that there will be a lot of intermediate conflicts while rebasing although the final diff will not have any conflicts...)

One trick I use (particuarly if I do not super care about the history or am willing to re-create it later) is to squash everything to one commit and then rebase.

QuLogic · 2023-01-26T08:40:28Z

lib/matplotlib/animation.py

+                )
+        if self._save_count is None and cache_frame_data:
+            _api.warn_external(
+                f"{frames=!r} which we can infer the length of, "


Is this supposed to say "which we cannot infer the length of"?

oscargus added API: changes topic: animation labels Oct 8, 2022

oscargus added this to the v3.7.0 milestone Oct 8, 2022

oscargus force-pushed the animationdeprecations branch from 7c7201c to 173e3c1 Compare October 8, 2022 19:57

oscargus marked this pull request as draft October 13, 2022 15:56

oscargus force-pushed the animationdeprecations branch 3 times, most recently from 09e467c to 11de658 Compare October 15, 2022 10:33

oscargus force-pushed the animationdeprecations branch 2 times, most recently from 38434b9 to bfec3c4 Compare October 15, 2022 14:56

oscargus mentioned this pull request Oct 30, 2022

[Doc]: Axes limits not updated in animate decay #24296

Closed

github-actions bot added the status: needs rebase label Nov 1, 2022

tacaswell force-pushed the animationdeprecations branch from 04e4255 to 1442dc4 Compare December 15, 2022 19:36

github-actions bot removed the status: needs rebase label Dec 15, 2022

tacaswell marked this pull request as ready for review December 15, 2022 19:37

oscargus and others added 5 commits December 16, 2022 16:49

Deprecate attributes and expire deprecation in animation

ec60128

MNT: add warnings if we ignore user passed save_count

96fcb93

MNT: add warning if we disable cache_frame_data

dabd667

This prevents a possible run-away unbounded cache!

MNT: if we are caching frames then we have a save_count

384b1eb

TST: add tests for warnings and avoid warnings in tests

bbeb86f

tacaswell force-pushed the animationdeprecations branch from 1442dc4 to bbeb86f Compare December 16, 2022 21:49

tacaswell approved these changes Dec 16, 2022

View reviewed changes

DOC: set save count in 3 animation examples

790c743

timhoffm reviewed Dec 16, 2022

View reviewed changes

doc/api/next_api_changes/behavior/24131-OG.rst Outdated Show resolved Hide resolved

lib/matplotlib/tests/test_animation.py Show resolved Hide resolved

.circleci/config.yml Outdated Show resolved Hide resolved

oscargus commented Dec 17, 2022

View reviewed changes

lib/matplotlib/animation.py Outdated Show resolved Hide resolved

QuLogic reviewed Dec 21, 2022

View reviewed changes

oscargus and others added 2 commits December 21, 2022 17:11

Fixup

13f398e

Update doc/api/next_api_changes/behavior/24131-OG.rst

d483a96

Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>

tacaswell force-pushed the animationdeprecations branch from acf484c to d483a96 Compare December 21, 2022 22:12

DOC: fix markup

046d0c2

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>

Fix doc-build

a253f22

oscargus added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Dec 27, 2022

greglucas approved these changes Dec 29, 2022

View reviewed changes

greglucas merged commit c9fc6a3 into matplotlib:main Dec 29, 2022

oscargus deleted the animationdeprecations branch December 31, 2022 09:57

QuLogic reviewed Jan 26, 2023

View reviewed changes

QuLogic mentioned this pull request Jan 26, 2023

FuncAnimation with generator defaults to arbitrary save_count=100 #8278

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate attributes and expire deprecation in animation #24131

Deprecate attributes and expire deprecation in animation #24131

oscargus commented Oct 8, 2022

oscargus commented Oct 10, 2022 •

edited

Loading

QuLogic commented Oct 13, 2022

oscargus commented Oct 13, 2022

oscargus commented Oct 15, 2022

story645 commented Oct 30, 2022

QuLogic commented Nov 1, 2022

story645 commented Nov 1, 2022

oscargus commented Nov 7, 2022

tacaswell commented Dec 15, 2022

tacaswell commented Dec 15, 2022

tacaswell left a comment

oscargus commented Dec 17, 2022

oscargus commented Dec 17, 2022

greglucas commented Dec 17, 2022

QuLogic Dec 21, 2022

tacaswell Dec 21, 2022

tacaswell commented Dec 21, 2022

tacaswell commented Dec 21, 2022

oscargus commented Dec 27, 2022

oscargus commented Dec 27, 2022

tacaswell commented Jan 5, 2023

QuLogic Jan 26, 2023

		ani = animation.FuncAnimation(fig, run, data_gen, interval=100, init_func=init,
		save_count=100)

Deprecate attributes and expire deprecation in animation #24131

Deprecate attributes and expire deprecation in animation #24131

Conversation

oscargus commented Oct 8, 2022

PR Summary

PR Checklist

oscargus commented Oct 10, 2022 • edited Loading

QuLogic commented Oct 13, 2022

oscargus commented Oct 13, 2022

oscargus commented Oct 15, 2022

story645 commented Oct 30, 2022

QuLogic commented Nov 1, 2022

story645 commented Nov 1, 2022

oscargus commented Nov 7, 2022

tacaswell commented Dec 15, 2022

tacaswell commented Dec 15, 2022

tacaswell left a comment

Choose a reason for hiding this comment

oscargus commented Dec 17, 2022

oscargus commented Dec 17, 2022

greglucas commented Dec 17, 2022

QuLogic Dec 21, 2022

Choose a reason for hiding this comment

tacaswell Dec 21, 2022

Choose a reason for hiding this comment

tacaswell commented Dec 21, 2022

tacaswell commented Dec 21, 2022

oscargus commented Dec 27, 2022

oscargus commented Dec 27, 2022

tacaswell commented Jan 5, 2023

QuLogic Jan 26, 2023

Choose a reason for hiding this comment

oscargus commented Oct 10, 2022 •

edited

Loading