matplotlib-devel

[matplotlib-devel] Animation class: let save() accept **kwargs which are passed on to savefig()?

[Maximilian A. <max...@gm...>]

[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

Re: [matplotlib-devel] Animation class: let save() accept **kwargs which are passed on to savefig()?

[Damon M. <dam...@gm...>]

On Wed, Oct 31, 2012 at 7:04 AM, Maximilian Albert
<max...@gm...> 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

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

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

Re: [matplotlib-devel] Animation class: let save() accept **kwargs which are passed on to savefig()?

[Maximilian A. <max...@gm...>]

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

Re: [matplotlib-devel] Animation class: let save() accept **kwargs which are passed on to savefig()?

[Damon M. <dam...@gm...>]

On Wed, Oct 31, 2012 at 7:40 AM, Maximilian Albert
<max...@gm...> 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

Re: [matplotlib-devel] Animation class: let save() accept **kwargs which are passed on to savefig()?

[Eric F. <ef...@ha...>]

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
> Mat...@li...
> https://lists.sourceforge.net/lists/listinfo/matplotlib-devel
>

Re: [matplotlib-devel] Animation class: let save() accept **kwargs which are passed on to savefig()?

[Jens N. <jen...@gm...>]

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 <ef...@ha...> 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
> > Mat...@li...
> > 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
> Mat...@li...
> https://lists.sourceforge.net/lists/listinfo/matplotlib-devel
>

Re: [matplotlib-devel] Animation class: let save() accept **kwargs which are passed on to savefig()?

[Maximilian A. <max...@gm...>]

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 <dam...@gm...>:
> On Wed, Oct 31, 2012 at 7:40 AM, Maximilian Albert
> <max...@gm...> 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

Re: [matplotlib-devel] Animation class: let save() accept **kwargs which are passed on to savefig()?

[Maximilian A. <max...@gm...>]

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 <max...@gm...>:
> 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 <dam...@gm...>:
>> On Wed, Oct 31, 2012 at 7:40 AM, Maximilian Albert
>> <max...@gm...> 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

Re: [matplotlib-devel] Animation class: let save() accept **kwargs which are passed on to savefig()?

[Ryan M. <rm...@gm...>]

On Thu, Nov 1, 2012 at 5:38 AM, Maximilian Albert
<max...@gm...> wrote:
> 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

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

Re: [matplotlib-devel] Animation class: let save() accept **kwargs which are passed on to savefig()?

[Maximilian A. <max...@gm...>]

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 <rm...@gm...>:
> 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. :)

> 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. :)

Best regards,
Max

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

Re: [matplotlib-devel] Animation class: let save() accept **kwargs which are passed on to savefig()?

[Maximilian A. <max...@gm...>]

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