endless looping GIFs with PillowWriter #11789

t-makaro · 2018-07-28T05:43:04Z

PR Summary

Closes #11787 . Adds an optional parameter to PillowWriter that will set the number of times that a GIF will loop for. Defaults to 0 meaning endless looping. Previously defaulted to 1 (never looping) without a way to change it. API change removed.

Makes PillowWriter produce endless looping gifs like ImageMagickWriter and ImageMagickFileWriter.

PR Checklist

~~[ ] Has Pytest style unit tests~~
Code is PEP 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
~~[ ] Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way~~
API consistency

t-makaro · 2018-07-28T05:50:50Z

Checklist 1: I have no idea how I would create a pytest for this.

Checklist 3,4: I want to add the loop parameter to the PillowWrite.__init__ docstring, but I believe that I would have to copy the entire docstring from MovieWriter.__init__, and then add the extra parameter, so that the sphinx-autodoc will work properly. That's that best way I can think of doing it, is it?

Checklist 5: It's a minor new feature. So, I shouldn't add this?
Checklist 6: It is an api change, but it shouldn't break anything? Like who relied on a gif that only looped once?

jklymak · 2018-07-28T13:34:16Z

can you interrogate the gif header for the loop parameter? Probably not so important for a test here.

3/4 Maybe just in the MovieWriter doc string say that loop exists if the writer is Pillow. For triple bonus points you could look into the other writers and see if there is a loop method for those.
5/6 I think a whats new entry is good, but no need for an API entry - thats generally only if API is changed, not if some is added.

WeatherGod · 2018-07-28T13:53:24Z

In some respects, this could be considered a bugfix. The original gif writer that uses ImageMagick produces endless loop GIFs. I don't think there is an API parameter for that writer as well.

…

On Sat, Jul 28, 2018 at 9:34 AM, Jody Klymak ***@***.***> wrote: 1. can you interrogate the gif header for the loop parameter? Probably not so important for a test here. 3/4 Maybe just in the MovieWriter doc string say that loop exists if the writer is Pillow. For triple bonus points you could look into the other writers and see if there is a loop method for those. 5/6 I think a whats new entry is good, but no need for an API entry - thats generally only if API is changed, not if some is added. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#11789 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARy-EvtAIRCtbVQ0avD7vZOmx1C2g9Vks5uLGhZgaJpZM4Vk6qM> .

jklymak · 2018-07-28T13:58:46Z

Oh, well in that case it’d be good to quickly go through the writers and document their behaviour.

t-makaro · 2018-07-28T16:39:50Z

I thought that I'd take a crack a setting the number of loops with ImageMagickWriter. I attempted to modify this line, but the gif still looped infinitely despite this doc. I'll stick with just modifying PillowWriter.

t-makaro · 2018-07-28T17:41:52Z

Warning, treated as error:
/home/circleci/project/doc/users/next_whats_new/endless_gif.rst:document isn't included in any toctree

I think this is just since the next_whats_new toctree is commented out in whats_new.rst

anntzer

anyone can dismiss once comments handled

anntzer · 2018-07-28T18:01:21Z

lib/matplotlib/animation.py

@@ -557,6 +557,7 @@ def isAvailable(cls):
    def __init__(self, *args, **kwargs):


__init__(self, *args, loop=False, **kwargs)
or at least make it a boolean.

Argument needs to be documented in some docstring.

Please check for API consistency across writers; if the behavior of other writers is to loop then the default should be changed here.

The Pillow image.save function takes the loop parameter as an integer, so a gif could be make to loop a set number of times. Also, loop=False might make sense in the docstring, but loop=self.loop where self.loop is False is an endless loop since False==0. I suppose I could change it to loop=not self.loop, but should we not allow setting to loop say 5 times?

I'll change it to a Boolean if you still want. (I suppose no one needs a gif to loop exactly 137 times!)

I like the move to the loop kwarg in the __init__ function definition. I'll do that for sure.

HTMLWriter and ImageMagick both default to looping I believe, but the ffmpegwriter writing to a .mp4 wouldn't loop. Whether defaulting to loop or not really depends on the format. Gif's should loop, mp4's can't. Different formats have different looping properties (HTMLWriter has a 'reflect' option, so it has a default_mode parameter that takes a string instead of a loop parameter.

I'll default it to looping (unless you object).

I know that I need to put it in a docstring, but I'm having difficulty figuring out where. I'm not familiar with sphinx-autodoc. I really could use help here. I could write an entire new docstring based off the base class and add the parameter. Or I could do as @jklymak suggested, but then other writers would have a "loop parameter only available in PillowWriter".

anntzer · 2018-07-28T18:01:37Z

doc/users/next_whats_new/endless_gif.rst

@@ -0,0 +1,7 @@
+Looping GIFs with PillowWriter


Something went wrong with the formatting.

I think the file has windows line endings. I will change that. Other than that I don't know.

jklymak · 2018-07-28T19:10:54Z

I think you could accept a Boolean and convert to an integer internally; just check if a Boolean. OTOH it’s a bit confusing that 0 is loop and 1 is loop only once. Agree that specifying the number loops is desiarable option

t-makaro · 2018-07-28T19:20:01Z

I suppose I could do

self.loop = not loop if isintance(loop, bool) else loop

then set the default to loop=True. Then in whatever docstring say that it accepts a Boolean or an integer that specifies number of loops. This way it is clear that True is endless, False is once, and an integer >=1 is an integer.

What do people think?

jklymak · 2018-07-28T19:29:42Z

More that I think about it maybe just make it an integer and value error on anything else so the Boolean doesn’t get passed by accident. Maybe just lay out the options here and folks can vote 🗳 (I’d do it but am on phone). Sorry to drag out a simple change but may as well get it right.

t-makaro · 2018-07-28T19:48:31Z

Option 1:
Booleans only.
Pros:

Simple.

Cons:

can't set a finite >= 1 integer of loops to display.

Implemented as

self.loop = not loop

Option 2:
ints (this is what it currently is, but without the ValueError)
Pros:

Can set a finite >= 1 integer of loops to display.

Cons:

0 == infinity

Implemented as

# can't use isinstance since bools are ints
if type(loop) == int:
    self.loop = loop
else:
    raise ValueError("loop must be an int")

Option 3:
Both bools and ints
Pros:

simple True is endless and False plays once
finite number of loops is possible

Cons:

We have to explain that both are excepted.

Implemented as

self.loop = not loop if isinstance(loop, bool) else loop

Documentation will be updated to reflect whatever choice.
I'm in favour of option 2 or 3.

jklymak · 2018-07-28T20:12:58Z

I vote 2 + ValueError, just because I think loop=False will actually pass loop=0 if its not caught, but I may be wrong.

t-makaro · 2018-07-28T21:17:04Z

I added the ValueError. If we did want to support both bools and ints in the future, then it is easier to add that, then it is to remove support for bools after the fact.

dopplershift

👍 on the functionality. Minor tweak to the int check if you're interested.

dopplershift · 2018-07-31T23:17:15Z

lib/matplotlib/animation.py

@@ -554,7 +554,38 @@ def isAvailable(cls):
            return False
        return True

-    def __init__(self, *args, **kwargs):
+    def __init__(self, *args, loop=0, **kwargs):


So much easier to do this now that we're Python 3 only.

dopplershift · 2018-07-31T23:17:33Z

lib/matplotlib/animation.py

+            output file. Some keys that may be of use include:
+            title, artist, genre, subject, copyright, srcform, comment.
+        '''
+        if type(loop) == int:


if not isinstance(loop, int): raise ValueError(...) self.loop = loop

?

bools are ints. So not isinstance(True, int) will return false. Which will result in passing a value of True aka 1 to loop which won't loop, but one would expect loop=True to loop. See my option 3 for a way to implement support for both bools and ints, but this will force just ints.

Though I suppose

if type(loop) != int: raise ValueError(...) self.loop=loop

But it only saves one of code, and I don't really feel like pushing another commit to this unless I have too 😛.

dopplershift · 2018-07-31T23:20:44Z

One other question on further thought: since this is not a boolean flag, but a number of loops, should the parameter be loops rather than loop?

t-makaro · 2018-07-31T23:38:22Z

That's not a bad point. But I think that the plural loops makes the fact that a value of 0 results in endless looping make even less sense. Pillow uses the parameter name loop, maybe we should just stick with that.

fredrik-1 · 2018-08-12T09:56:17Z

I am looking through the animation module at the moment and based on the available options in all writers at the moment I would say that we could just add unlimited looping also in the pillow writer without any option to change it. Most people want it anyway (but I am not against the option).

I think that a refractoring or at least change in documentation about kwargs should be done in the animation module. At the moment the parameters to the pillow writer are something like,
codec, fps, bitrate, extra_args, metadata but the only one that are not ignored is fps.

So a new API or documentation that clearly show what options that are used in a writer and adding new relevant options should be good.

I have started to change some of the documentation to show which kwargs that are actually used and how.

I would suggest to just add the unlimited looping functionality in a pr and the rest in other prs. Do this really need a test?

fredrik-1 · 2018-08-12T09:58:53Z

lib/matplotlib/animation.py

+        loop: int
+            The number of times that the gif will loop.
+            A value of 0 is endless.
+        codec: string or None, optional


The rest of the options are just ignored in the pillow writer so I don't think that they should be documented.

fredrik-1 · 2018-08-12T15:59:36Z

I thought that I'd take a crack a setting the number of loops with ImageMagickWriter. I attempted to modify this line, but the gif still looped infinitely despite this doc. I'll stick with just modifying PillowWriter.

Changing that line worked for me. I think that both the pillow- and imagemagick writer API should be the same.

One possibility that are not perfect but is in accordance with the current implementation is to put the new kwarg in moviewriter and just put better documentation in the subclasses which kwargs that are used in that writer.

When both imagemagick and pillow seems to use ints for number of loops with 0 as infinity I think that we should stick to that notation.

Edit:
I would even say that this pr and the extra kwargs in the HTMLWriter breaks the API. It is with those changes not possible to get the same functionality from anim.save(name, writer=writer_obj, ...) and anim.save(name, writer=string_name_of_writer, ...).

So I believe that all new kwargs should be in MovieWriter and animation.save

t-makaro · 2018-09-18T23:10:25Z

Sorry it's been a while.

In the interest in getting this bug fix done (and leaving me time for my thousand other projects), I've removed all API changes. This PR now does 2 things:

Makes gifs with PillowWriter loop endlessly. Consistent with ImageMagickWriter and ImageMagickFileWriter.
~~Adds a simple docstring to PillowWriter.~~

I'll leave any refractoring/API changes to someone more familiar with this API than me. This PR is now the minimum needed to fix #11789. The suggested loop keyword argument can be added later.

anntzer · 2018-09-19T08:50:58Z

lib/matplotlib/animation.py

+        ----------
+        fps: int
+            Framerate for the gif.
+        '''


kill the docstring as well for now?

anntzer · 2018-09-19T08:52:40Z

doc/users/next_whats_new/endless_gif.rst

+Endless Looping GIFs with PillowWriter
+--------------------------------------
+
+We acknowledge that most people want to watch a gif more than once.  Saving an animatation


typo (animatation)

Silly me. Fixed.

anntzer

modulo typo in whatsnew

@dopplershift

@dopplershift I dismissed your review as the scope of the PR has changed; can you check it again?

t-makaro · 2018-09-21T00:49:53Z

Ok, I fixed the typo. I believe the doc failures are due to sphinx 1.8.0.

QuLogic · 2018-09-29T01:37:29Z

We should have fixed the doc builds now; can you rebase to see if everything's working again?

t-makaro · 2018-10-03T19:57:20Z

Last time I tried to rebase a fork, I really messed up the commit history. Any advice?

jklymak · 2018-10-03T20:24:37Z

You should have branched your fork before submitting this PR. Right now your PR is based on your master branch, and that will make your life hard.

After that, you'd best give https://matplotlib.org/devel/gitwash/index.html a thorough look. In particular pay attention to backing up your branch.

With your setup, I would do:

git checkout master
git checkout -b backup-of-master  # this will save your bacon if you mess up.
git checkout master    # get back to the changes you made in this PR
git fetch upstream   # if you haven't set your 'upstream' remote, then you need to do that first.  
git rebase upstream/master  # you don't have any conflicts so this should be smooth
git push origin master --force

If you mess up:

git checkout master
git reset --hard backup-of-master

will get you locally back to where you were.

QuLogic · 2018-10-03T20:28:07Z

This is some redundant branch switching:

git checkout -b backup-of-master # this will save your bacon if you mess up.
git checkout master # get back to the changes you made in this PR

git branch backup-of-master will create a branch and not switch to it.

Adds an optional parameter to PillowWriter that will set the number of times that a gif should loop for. Defaults to 0 meaning endless

t-makaro · 2018-10-09T17:21:21Z

@jklymak Your advice seems to have worked. Thank you. I've rebased.

dstansby · 2018-10-16T15:20:11Z

Can whoever merges this squash-merge it?

* master: (51 commits) Disable sticky edge accumulation if no autoscaling. Avoid quadratic behavior when accumulating stickies. Rectified plot error (matplotlib#12501) endless looping GIFs with PillowWriter (matplotlib#11789) TST: add test for re-normalizing image FIX: colorbar re-check norm before draw for autolabels Fix search for sphinx >=1.8 (matplotlib#12216) Fix some flake8 issues Don't handle impossible values for `align` in hist() DOC: add section about test / doc dependencies DOC: clarify time windows for support TST: test colorbar tick labels correct FIX: make minor ticks formatted with science formatter as well DOC: clarify what 'minor version' means Adjust the widths of the messages during the build. Simplify radar_chart example. Fix duplicate condition in pathpatch3d example (matplotlib#12495) Fix typo in documentation FIX: make unused spines invisible Kill FontManager.update_fonts. ...

anntzer requested changes Jul 28, 2018

View reviewed changes

tacaswell added this to the v3.1 milestone Jul 28, 2018

dstansby added the topic: animation label Jul 30, 2018

dopplershift previously approved these changes Jul 31, 2018

View reviewed changes

fredrik-1 reviewed Aug 12, 2018

View reviewed changes

anntzer reviewed Sep 19, 2018

View reviewed changes

anntzer approved these changes Sep 19, 2018

View reviewed changes

timhoffm added the status: needs rebase label Oct 3, 2018

t-makaro and others added 8 commits October 9, 2018 10:18

add loop kwarg to PillowWriter

b953a82

Adds an optional parameter to PillowWriter that will set the number of times that a gif should loop for. Defaults to 0 meaning endless

next_whats_new file for endless gifs

43e1a7f

adjust PillowWriter loop kwarg

fe87711

add docstring to PillowWriter

be2e2af

ValueError if loop is not an int in PillowWriter

dae6ea6

remove API changes

5484bc7

fix typo

2dc2c99

kill docstring for now

50568c7

dstansby approved these changes Oct 16, 2018

View reviewed changes

dstansby removed the status: needs rebase label Oct 16, 2018

jklymak merged commit 566bd8b into matplotlib:master Oct 16, 2018

znstrider mentioned this pull request Dec 23, 2020

PillowWriter GIF duration and loop #19169

Closed

		@@ -557,6 +557,7 @@ def isAvailable(cls):
		def __init__(self, args, *kwargs):

endless looping GIFs with PillowWriter #11789

endless looping GIFs with PillowWriter #11789

Conversation

t-makaro commented Jul 28, 2018 • edited Loading

PR Summary

PR Checklist

t-makaro commented Jul 28, 2018 • edited Loading

jklymak commented Jul 28, 2018

WeatherGod commented Jul 28, 2018 via email

jklymak commented Jul 28, 2018

t-makaro commented Jul 28, 2018

t-makaro commented Jul 28, 2018

anntzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-makaro Jul 28, 2018 • edited Loading

Choose a reason for hiding this comment

jklymak commented Jul 28, 2018

t-makaro commented Jul 28, 2018 • edited Loading

jklymak commented Jul 28, 2018

t-makaro commented Jul 28, 2018 • edited Loading

jklymak commented Jul 28, 2018

t-makaro commented Jul 28, 2018

dopplershift left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dopplershift commented Jul 31, 2018

t-makaro commented Jul 31, 2018

fredrik-1 commented Aug 12, 2018

Choose a reason for hiding this comment

fredrik-1 commented Aug 12, 2018 • edited Loading

t-makaro commented Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer left a comment

Choose a reason for hiding this comment

t-makaro commented Sep 21, 2018

QuLogic commented Sep 29, 2018

t-makaro commented Oct 3, 2018

jklymak commented Oct 3, 2018

QuLogic commented Oct 3, 2018

t-makaro commented Oct 9, 2018

dstansby commented Oct 16, 2018

t-makaro commented Jul 28, 2018 •

edited

Loading

t-makaro commented Jul 28, 2018 •

edited

Loading

t-makaro Jul 28, 2018 •

edited

Loading

t-makaro commented Jul 28, 2018 •

edited

Loading

t-makaro commented Jul 28, 2018 •

edited

Loading

fredrik-1 commented Aug 12, 2018 •

edited

Loading

t-makaro commented Sep 18, 2018 •

edited

Loading