Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Add savefig_kwargs to Animation.save() method #1442

Merged
merged 3 commits into from

4 participants

@maxalbert

Allow Animation.save() to accept keyword arguments which are passed on to savefig() when saving individual frames (this can be used to set tight bounding boxes, for example).

@maxalbert maxalbert Allow Animation.save() to accept keyword arguments which are passed o…
…n to savefig() when saving individual frames. This can be used to set tight bounding boxes etc.
9a48efd
@jenshnielsen
Owner

Looks fine. I might have misguided you a bit on the mailing list by suggesting that you looked at my branch (#1420). As @WeatherGod pointed out it is not a good idea to have a mutable type (in this case an empty dict) as the call signature default.
See commit for an 63733aa for an alternative solution and http://www.siafoo.net/article/52#id41 for an explanation why. Otherwise this look very good.

@maxalbert maxalbert Don't use a mutable type (here, a dict) as default argument because i…
…t only gets evaluated once when the function is defined, not during each function call (i.e. changes will persist across function calls).
5f42166
@maxalbert

Thanks very much for this review, @jenshnielsen . I wasn't aware of the danger that mutable types present when used as default arguments, and just pushed a fix for this (see 5f42166). Regarding commit 63733aa which you referenced, perhaps I'm missing something but I don't see how this issue is fixed there since the dict() that it uses as the default value is still mutable. Anyway, many thanks for pointing this out!

@jenshnielsen
Owner

Sorry wrong comit it should have been 6d585f3 which is identical to what you just did. IMHO this looks good now and should be ready to merge

@maxalbert

Ah, that makes more sense. :) Thanks again for your quick and helpful responses!

@maxalbert

P.S.: I just discovered a small bug which is introduced/exposed by this change in the Animation class. I'll ask about this on the mailing list, but perhaps it's best to wait for comments there before merging (or perhaps it can even be fixed easily).

@WeatherGod
Collaborator

Fixing the bbox_inches issue is going to be tricky. bbox_inches is probably one of the most commonly used kwargs in savefig, and most people are going to expect it to work. The problem is that it if bbox_inches='tight' (the most common usage, IMO), then bbox_inches='tight' is applied to each and every frame of the animation, which may or may not change. We could easily define that the tight bounding box would be derived from the first frame, and that may be sufficient.

The other difficulty that may arise here is the order of operations. We would have to know the figure size prior to initializing the mencoder or ffmpeg stream, but a tight bbox can't be known until the first frame is drawn. This may require a little bit of tweaking in the animation saving code.

Because bbox_inches is such an important kwarg for savefig, I would suggest either holding off on merging for a bit, or to include a warning in the save() docstring that the bbox_inches='tight' option is currently ignored (and to pop it out if it exists in savefig_kwargs).

@maxalbert

Thanks for the suggestions, @WeatherGod . I have now added a check to drop the 'bbox_inches' argument if a writer is being used that doesn't support it (and issue a warning in this case). I hope this resolves the issue sufficiently to make the branch merge-able (because 'bbox_inches' is useable with some writers, so no functionality is too severely limited).

@dopplershift dopplershift was assigned
@dopplershift
Collaborator

I like the warning. I'm not sure if we should be whitelisting only mencoder_file and ffmpeg_file, or if we should just blacklist their pipe-based counterparts. It's relevant because we also have the (newly added) ImageMagick writers. I don't have an animation with a changing bounding box, so I can't really test if either (or both) ImageMagick writers will work--I suspect they both will. Can someone test?

@maxalbert

I tested the 'imagemagick_file' writer yesterday before uploading the new commit, but it didn't work (I even got an error message), so I didn't whitelist it. I have now also tried the 'imagemagick' writer. I don't get an error, but the animation file it creates has size zero. I tried to take a look into why this happens but don't really have the time to debug at the moment. It seems to me that it's safer to whitelist writers that are kown to work instead of accidentally forgetting to blacklist a broken writer. We could merge the change as it is (if there are no other objections) and also whitelist the imagemagick writers once they work. Any other opinions?

@maxalbert

So, there haven't been any comments on this for a while. It seems to me that this is theoretically ready to merge, unless I missed any fundamental objections. The only open question was whether we should whitelist or blacklist writers for use with the bbox_inches argument. At the moment I have whitelisted the writers ffmpeg_file and mencoder_file only because AFAICS these are the only ones that work. I tested with both imagemagick and imagemagick_file, too, but neither of these works at the moment (see my previous comment). So it see seems to me that whitelisting is currently the better option. We can always change it down the line if we get other writers to work. Any objections against a merge at this point?

@dopplershift dopplershift merged commit 8b13952 into from
@dopplershift
Collaborator

I kept meaning to test imagemagick, but haven't found the time. I'll just merge.

@maxalbert

Thanks! Yes, it's strange that the imagemagick writer gives me zero file size. Unfortunately, I won't have the time to debug this anytime soon, but I'll keep it in mind in case I find a few spare hours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 31, 2012
  1. @maxalbert

    Allow Animation.save() to accept keyword arguments which are passed o…

    maxalbert authored
    …n to savefig() when saving individual frames. This can be used to set tight bounding boxes etc.
Commits on Nov 1, 2012
  1. @maxalbert

    Don't use a mutable type (here, a dict) as default argument because i…

    maxalbert authored
    …t only gets evaluated once when the function is defined, not during each function call (i.e. changes will persist across function calls).
Commits on Nov 5, 2012
  1. @maxalbert
This page is out of date. Refresh to see the latest.
Showing with 32 additions and 4 deletions.
  1. +32 −4 lib/matplotlib/animation.py
View
36 lib/matplotlib/animation.py
@@ -184,9 +184,11 @@ def finish(self):
'Finish any processing for writing the movie.'
self.cleanup()
- def grab_frame(self):
+ def grab_frame(self, **savefig_kwargs):
'''
Grab the image information from the figure and save as a movie frame.
+ All keyword arguments in savefig_kwargs are passed on to the 'savefig'
+ command that saves the figure.
'''
verbose.report('MovieWriter.grab_frame: Grabbing frame.',
level='debug')
@@ -194,7 +196,7 @@ def grab_frame(self):
# Tell the figure to save its data to the sink, using the
# frame format and dpi.
self.fig.savefig(self._frame_sink(), format=self.frame_format,
- dpi=self.dpi)
+ dpi=self.dpi, **savefig_kwargs)
except RuntimeError:
out, err = self._proc.communicate()
verbose.report('MovieWriter -- Error running proc:\n%s\n%s' % (out,
@@ -545,7 +547,8 @@ def _stop(self, *args):
self.event_source = None
def save(self, filename, writer=None, fps=None, dpi=None, codec=None,
- bitrate=None, extra_args=None, metadata=None, extra_anim=None):
+ bitrate=None, extra_args=None, metadata=None, extra_anim=None,
+ savefig_kwargs=None):
'''
Saves a movie file by drawing every frame.
@@ -586,7 +589,32 @@ def save(self, filename, writer=None, fps=None, dpi=None, codec=None,
`matplotlib.Figure` instance. Also, animation frames will just be
simply combined, so there should be a 1:1 correspondence between
the frames from the different animations.
+
+ *savefig_kwargs* is a dictionary containing keyword arguments to be
+ passed on to the 'savefig' command which is called repeatedly to save
+ the individual frames. This can be used to set tight bounding boxes,
+ for example.
'''
+ if savefig_kwargs is None:
+ savefig_kwargs = {}
+
+ # FIXME: Using 'bbox_inches' doesn't currently work with writers that pipe
+ # the data to the command because this requires a fixed frame size (see
+ # Ryan May's reply in this thread: [1]). Thus we drop the 'bbox_inches'
+ # argument if it exists in savefig_kwargs.
+ #
+ # [1] http://matplotlib.1069221.n5.nabble.com/Animation-class-let-save-accept-kwargs-which-are-passed-on-to-savefig-td39627.html
+ #
+ if savefig_kwargs.has_key('bbox_inches'):
+ if not (writer in ['ffmpeg_file', 'mencoder_file'] or
+ isinstance(writer, (FFMpegFileWriter, MencoderFileWriter))):
+ print("Warning: discarding the 'bbox_inches' argument in " \
+ "'savefig_kwargs' as it is only currently supported " \
+ "with the writers 'ffmpeg_file' and 'mencoder_file' " \
+ "(writer used: '{}').".format(writer if isinstance(writer, str)
+ else writer.__class__.__name__))
+ savefig_kwargs.pop('bbox_inches')
+
# Need to disconnect the first draw callback, since we'll be doing
# draws. Otherwise, we'll end up starting the animation.
if self._first_draw_id is not None:
@@ -645,7 +673,7 @@ def save(self, filename, writer=None, fps=None, dpi=None, codec=None,
for anim, d in zip(all_anim, data):
#TODO: Need to see if turning off blit is really necessary
anim._draw_next_frame(d, blit=False)
- writer.grab_frame()
+ writer.grab_frame(**savefig_kwargs)
# Reconnect signal for first draw if necessary
if reconnect_first_draw:
Something went wrong with that request. Please try again.