Animation class: let save() accept **kwargs which are passed on to savefig()?

[I sent this email a few weeks ago already, but I wasn't subscribed to
matplotlib-devel at the time and it seems that the message was never
approved by the moderator. So here comes my second attempt. :)]

Hi all,

this is my first post to this mailing list, so let me take the
opportunity to thank everyone involved for an amazing piece of
software and all the hard work you guys put into it! It is very much
appreciated indeed.

I have a quick question/suggestion regarding the save() method in the
matplotlib.animation.Animation class. I recently produced an animation
in which I needed to set tight bounding boxes when saving the
individual frames. Obviously savefig() supports this, but there is no
way to pass this information to the Animation.save() method. Would it
make sense to let Animation.save() accept additional keyword arguments
which are simply passed on to savefig() in each step of the animation
loop? Or am I overlooking potential drawbacks of this approach? A
simple patch with this idea is attached. Feel free to use it as is or
to modify at will if you think this is useful.

Many thanks and kind regards,
Max

0001-Allow-Animation.save-to-accept-keyword-arguments-whi.patch (2.96 KB)

Hi Max,

Sounds like a great idea! Would you feel comfortable having a go at an
implementation? You can make a pull request out of it. The rest of the
developers can then deliberate and provide feedback for you.

Best wishes,
Damon

···

On Wed, Oct 31, 2012 at 7:04 AM, Maximilian Albert <maximilian.albert@...149...> wrote:

[I sent this email a few weeks ago already, but I wasn't subscribed to
matplotlib-devel at the time and it seems that the message was never
approved by the moderator. So here comes my second attempt. :)]

Hi all,

this is my first post to this mailing list, so let me take the
opportunity to thank everyone involved for an amazing piece of
software and all the hard work you guys put into it! It is very much
appreciated indeed.

I have a quick question/suggestion regarding the save() method in the
matplotlib.animation.Animation class. I recently produced an animation
in which I needed to set tight bounding boxes when saving the
individual frames. Obviously savefig() supports this, but there is no
way to pass this information to the Animation.save() method. Would it
make sense to let Animation.save() accept additional keyword arguments
which are simply passed on to savefig() in each step of the animation
loop? Or am I overlooking potential drawbacks of this approach? A
simple patch with this idea is attached. Feel free to use it as is or
to modify at will if you think this is useful.

Many thanks and kind regards,
Max

--
Damon McDougall
http://www.damon-is-a-geek.com
B2.39
Mathematics Institute
University of Warwick
Coventry
West Midlands
CV4 7AL
United Kingdom

Hi Damon,

many thanks for the quick (and positive :)) reply,

Sounds like a great idea! Would you feel comfortable having a go at an
implementation? You can make a pull request out of it. The rest of the
developers can then deliberate and provide feedback for you.

I attached a patch to my previous email which contains an
implementation, but perhaps it didn't make it through to the list? But
since I had to clone the git repository anyway to implement it, I'm
happy to make a pull request, too. I'll have to find out how to do
that since I've never done it before, but I guess it can't be too
difficult. Will give it a shot later today.

Thanks again and best regards,
Max

You need to fork the main matplotlib repo, so you have your own copy
associated with your github account:
https://help.github.com/articles/fork-a-repo

Once you've forked it, clone it and create a branch:

git clone my-forked-repo-url
cd matplotlib
git checkout -b my_awesome_new_feature
# ... hack hack hack ...
git commit -am "Useful commit message here"
git push origin my_awesome_new_feature

Once you've done that, make a pull request by following the
instructions here:
https://help.github.com/articles/using-pull-requests

Once you've done that, congratulations!

Hope this helps.
Best wishes,
Damon

···

On Wed, Oct 31, 2012 at 7:40 AM, Maximilian Albert <maximilian.albert@...149...> wrote:

Hi Damon,

many thanks for the quick (and positive :)) reply,

Sounds like a great idea! Would you feel comfortable having a go at an
implementation? You can make a pull request out of it. The rest of the
developers can then deliberate and provide feedback for you.

I attached a patch to my previous email which contains an
implementation, but perhaps it didn't make it through to the list? But
since I had to clone the git repository anyway to implement it, I'm
happy to make a pull request, too. I'll have to find out how to do
that since I've never done it before, but I guess it can't be too
difficult. Will give it a shot later today.

--
Damon McDougall
http://www.damon-is-a-geek.com
B2.39
Mathematics Institute
University of Warwick
Coventry
West Midlands
CV4 7AL
United Kingdom

[I sent this email a few weeks ago already, but I wasn't subscribed to
matplotlib-devel at the time and it seems that the message was never
approved by the moderator. So here comes my second attempt. :)]

Hi all,

this is my first post to this mailing list, so let me take the
opportunity to thank everyone involved for an amazing piece of
software and all the hard work you guys put into it! It is very much
appreciated indeed.

I have a quick question/suggestion regarding the save() method in the
matplotlib.animation.Animation class. I recently produced an animation
in which I needed to set tight bounding boxes when saving the
individual frames. Obviously savefig() supports this, but there is no
way to pass this information to the Animation.save() method. Would it
make sense to let Animation.save() accept additional keyword arguments
which are simply passed on to savefig() in each step of the animation
loop? Or am I overlooking potential drawbacks of this approach? A
simple patch with this idea is attached. Feel free to use it as is or
to modify at will if you think this is useful.

I don't have time to look at this, so I will toss out one idea for consideration:

If there is any chance that other sorts of kwarg collections might be needed, or simply to improve readability and explicitness, instead of passing on **kwargs, you might make a new kwarg, "savefigkw", which would take a dictionary that would then be used via "savefig(..., **savefigkw".

Eric

···

On 2012/10/31 2:04 AM, Maximilian Albert wrote:

Many thanks and kind regards,
Max

------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_sfd2d_oct

_______________________________________________
Matplotlib-devel mailing list
Matplotlib-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/matplotlib-devel

I think Eric idea is a good solution. This is just to point out that I did something similar
with kw args to savefig in the image comparison decorator for tests. See the changes in
decorators.py in this pull request https://github.com/matplotlib/matplotlib/pull/1420 . Seems to work fine.

Greetings, Jens

···

On Wed, Oct 31, 2012 at 4:22 PM, Eric Firing <efiring@…229…> wrote:

On 2012/10/31 2:04 AM, Maximilian Albert wrote:

[I sent this email a few weeks ago already, but I wasn’t subscribed to

matplotlib-devel at the time and it seems that the message was never

approved by the moderator. So here comes my second attempt. :)]

Hi all,

this is my first post to this mailing list, so let me take the

opportunity to thank everyone involved for an amazing piece of

software and all the hard work you guys put into it! It is very much

appreciated indeed.

I have a quick question/suggestion regarding the save() method in the

matplotlib.animation.Animation class. I recently produced an animation

in which I needed to set tight bounding boxes when saving the

individual frames. Obviously savefig() supports this, but there is no

way to pass this information to the Animation.save() method. Would it

make sense to let Animation.save() accept additional keyword arguments

which are simply passed on to savefig() in each step of the animation

loop? Or am I overlooking potential drawbacks of this approach? A

simple patch with this idea is attached. Feel free to use it as is or

to modify at will if you think this is useful.

I don’t have time to look at this, so I will toss out one idea for

consideration:

If there is any chance that other sorts of kwarg collections might be

needed, or simply to improve readability and explicitness, instead of

passing on **kwargs, you might make a new kwarg, “savefigkw”, which

would take a dictionary that would then be used via "savefig(…,

**savefigkw".

Eric

Many thanks and kind regards,

Max


Everyone hates slow websites. So do we.

Make your web apps faster with AppDynamics

Download AppDynamics Lite for free today:

http://p.sf.net/sfu/appdyn_sfd2d_oct


Matplotlib-devel mailing list

Matplotlib-devel@lists.sourceforge.net

https://lists.sourceforge.net/lists/listinfo/matplotlib-devel


Everyone hates slow websites. So do we.

Make your web apps faster with AppDynamics

Download AppDynamics Lite for free today:

http://p.sf.net/sfu/appdyn_sfd2d_oct


Matplotlib-devel mailing list

Matplotlib-devel@lists.sourceforge.net

https://lists.sourceforge.net/lists/listinfo/matplotlib-devel

Awesome, many thanks for the detailed instructions, as well as for the
valuable suggestions by Eric and Jens. I have now created a pull
request containing the proposed change, including the suggested
modifications:

   https://github.com/matplotlib/matplotlib/pull/1442

Any comments/feedback would be much appreciated.

Best wishes,
Max

2012/10/31 Damon McDougall <damon.mcdougall@...149...>:

···

On Wed, Oct 31, 2012 at 7:40 AM, Maximilian Albert > <maximilian.albert@...149...> wrote:

Hi Damon,

many thanks for the quick (and positive :)) reply,

Sounds like a great idea! Would you feel comfortable having a go at an
implementation? You can make a pull request out of it. The rest of the
developers can then deliberate and provide feedback for you.

I attached a patch to my previous email which contains an
implementation, but perhaps it didn't make it through to the list? But
since I had to clone the git repository anyway to implement it, I'm
happy to make a pull request, too. I'll have to find out how to do
that since I've never done it before, but I guess it can't be too
difficult. Will give it a shot later today.

You need to fork the main matplotlib repo, so you have your own copy
associated with your github account:
https://help.github.com/articles/fork-a-repo

Once you've forked it, clone it and create a branch:

git clone my-forked-repo-url
cd matplotlib
git checkout -b my_awesome_new_feature
# ... hack hack hack ...
git commit -am "Useful commit message here"
git push origin my_awesome_new_feature

Once you've done that, make a pull request by following the
instructions here:
https://help.github.com/articles/using-pull-requests

Once you've done that, congratulations!

Hope this helps.
Best wishes,
Damon

--
Damon McDougall
http://www.damon-is-a-geek.com
B2.39
Mathematics Institute
University of Warwick
Coventry
West Midlands
CV4 7AL
United Kingdom

Hi all,

quick update on this: I pushed a small change to make the default
argument immutable (thanks to Jens for pointing this out). Just a
couple more questions/comments:

1) Should there be a test for this? I couldn't find any tests for the
Animation class, so I haven't added one. But perhaps I just missed
them.

2) I discovered this morning that my change uncovers/introduces a bug
in the Animation class, so I'd appreciate a bit more input on whether
it should be merged in the current state. Here is an explanation:

My original use case the suggested change was to be able to set tight
bounding boxes when saving animation frames. At the time I simply
saved all frames to separate images and combined them manually using
avconv, which worked fine. I saw that in the development version of
matplotlib there is built-in support for this, so that the video file
is created automatically. Now whenever I change the bounding box, e.g.
by passing something like savefig_kwargs={'bbox_inches': 'tight'} to
Animation.save(), then the output video shows complete garbage
(similar to white noise). I presume this is because the 'frame_size'
property in the MovieWriter class is not aware of the bounding box
changes introduced by savefig_kwargs and thus reports a frame size to
the video converter that is different from the actual size of the
saved frames.

I don't have much time to look into this at the moment, but I just
wanted to point it out. Does anyone have a quick idea for a good fix,
before I get the time to look into the details of how the MovieWriter
class works?

Many thanks,
Max

2012/10/31 Maximilian Albert <maximilian.albert@...149...>:

···

Awesome, many thanks for the detailed instructions, as well as for the
valuable suggestions by Eric and Jens. I have now created a pull
request containing the proposed change, including the suggested
modifications:

   https://github.com/matplotlib/matplotlib/pull/1442

Any comments/feedback would be much appreciated.

Best wishes,
Max

2012/10/31 Damon McDougall <damon.mcdougall@...149...>:

On Wed, Oct 31, 2012 at 7:40 AM, Maximilian Albert >> <maximilian.albert@...149...> wrote:

Hi Damon,

many thanks for the quick (and positive :)) reply,

Sounds like a great idea! Would you feel comfortable having a go at an
implementation? You can make a pull request out of it. The rest of the
developers can then deliberate and provide feedback for you.

I attached a patch to my previous email which contains an
implementation, but perhaps it didn't make it through to the list? But
since I had to clone the git repository anyway to implement it, I'm
happy to make a pull request, too. I'll have to find out how to do
that since I've never done it before, but I guess it can't be too
difficult. Will give it a shot later today.

You need to fork the main matplotlib repo, so you have your own copy
associated with your github account:
https://help.github.com/articles/fork-a-repo

Once you've forked it, clone it and create a branch:

git clone my-forked-repo-url
cd matplotlib
git checkout -b my_awesome_new_feature
# ... hack hack hack ...
git commit -am "Useful commit message here"
git push origin my_awesome_new_feature

Once you've done that, make a pull request by following the
instructions here:
https://help.github.com/articles/using-pull-requests

Once you've done that, congratulations!

Hope this helps.
Best wishes,
Damon

--
Damon McDougall
http://www.damon-is-a-geek.com
B2.39
Mathematics Institute
University of Warwick
Coventry
West Midlands
CV4 7AL
United Kingdom

Hi all,

quick update on this: I pushed a small change to make the default
argument immutable (thanks to Jens for pointing this out). Just a
couple more questions/comments:

1) Should there be a test for this? I couldn't find any tests for the
Animation class, so I haven't added one. But perhaps I just missed
them.

Sadly there are no tests. Partially because I'm lazy, and partially
because I'm not sure how exactly that would work. I'd gladly take a PR
with some if someone can figure it out, but I realize that's more than
you signed up for.

2) I discovered this morning that my change uncovers/introduces a bug
in the Animation class, so I'd appreciate a bit more input on whether
it should be merged in the current state. Here is an explanation:

My original use case the suggested change was to be able to set tight
bounding boxes when saving animation frames. At the time I simply
saved all frames to separate images and combined them manually using
avconv, which worked fine. I saw that in the development version of
matplotlib there is built-in support for this, so that the video file
is created automatically. Now whenever I change the bounding box, e.g.
by passing something like savefig_kwargs={'bbox_inches': 'tight'} to
Animation.save(), then the output video shows complete garbage
(similar to white noise). I presume this is because the 'frame_size'
property in the MovieWriter class is not aware of the bounding box
changes introduced by savefig_kwargs and thus reports a frame size to
the video converter that is different from the actual size of the
saved frames.

I don't have much time to look into this at the moment, but I just
wanted to point it out. Does anyone have a quick idea for a good fix,
before I get the time to look into the details of how the MovieWriter
class works?

You might have more luck using a temp-file based writer. By default,
movies are created by piping in the data to the command; this is much
faster, but, at least as I've done it now, requires a fixed number of
bytes per frame. Try passing writer='ffmpeg_file' or
writer='mencoder_file' to the command to save the animation.

If I get a chance (or someone else if you want to help), I'll see if
there's any way to make the pipe-based writers work with
variable-sized frames. Failing that, we could just ignore the tight
bbox option when using pipes for saving movies.

Thanks for the work!

Ryan

···

On Thu, Nov 1, 2012 at 5:38 AM, Maximilian Albert <maximilian.albert@...149...> wrote:

--
Ryan May
Graduate Research Assistant
School of Meteorology
University of Oklahoma

Hi all,

apologies for the delay in getting back to you! The end of last week
was quite busy and I was away from my computer during the weekend.

2012/11/1 Ryan May <rmay31@...149...>:

You might have more luck using a temp-file based writer. By default,
movies are created by piping in the data to the command; this is much
faster, but, at least as I've done it now, requires a fixed number of
bytes per frame. Try passing writer='ffmpeg_file' or
writer='mencoder_file' to the command to save the animation.

Yes, this works indeed. Thanks for pointing it out! I had feared that
getting the 'bbox_inches' argument to work at all would be much more
involved.

If I get a chance (or someone else if you want to help), I'll see if
there's any way to make the pipe-based writers work with
variable-sized frames.

That would be awesome of course. :slight_smile:

Failing that, we could just ignore the tight
bbox option when using pipes for saving movies.

I like this idea. I have now updated my branch so that the save()
method checks which writer is being used. If it is not a temp
file-based one a warning is issued and the 'bbox_inches' argument is
dropped if it is present (see [1]). Please review and comment. :slight_smile:

Best regards,
Max

[1] https://github.com/maxalbert/matplotlib/commit/fe44357d04fd708c616e88e386bb06100c12aaca

P.S.: As an aside, when I run ffmpeg on my machine it issues a
deprecation warning and suggests to use avconv instead. Is it worth
converting animation.py from ffmpeg to avconv, too? The command line
arguments should be virtually identicaly (as far as I know - I only
use it very occasionally, though).

Cheers,
Max