Fixed MPLRenderer anim output regression #2454
Conversation
philippjfr
added
type: bug
philippjfr
force-pushed
the
fix_mpl_anim_render
branch
from
960284f to
8b1f8b5
Compare
philippjfr
force-pushed
the
fix_mpl_anim_render
branch
from
c45f693 to
16e9406
Compare
philippjfr
force-pushed
the
fix_mpl_anim_render
branch
from
b4a2f4b to
5f00d18
Compare
|
Ready to merge. |
| "Note that GIF support requires [ImageMagick](http://www.imagemagick.org) which is installed by default on many Linux installations and may be installed on OSX using [brew](http://brew.sh) . For more information on how to install ImageMagick (including Windows instructions) see the [installation page](http://www.imagemagick.org/script/binary-releases.php)." | ||
| "### GIF\n", | ||
| "\n", | ||
| "Note that GIF support requires [ImageMagick](http://www.imagemagick.org) which is installed by default on many Linux installations and may be installed on OSX using [brew](http://brew.sh) . For more information on how to install ImageMagick (including Windows instructions) see the [installation page](http://www.imagemagick.org/script/binary-releases.php).\n", |
There was a problem hiding this comment.
I am tempted to deprecate the code path that uses imagemagick entirely. Installing imagemagick on systems where it isn't already available is a pain.
There was a problem hiding this comment.
matplotlib 2.2.0 with pillow support is only a few weeks old, so strong -1 from me on deprecating that code path now.
There was a problem hiding this comment.
I'm a bit confused; this PR says that it's using pillow rather than imagemagick, but the change above makes it sound like imagemagick is still needed? I too vote for never using imagemagick if we don't need to, as it's a non-python dependency that's surprising to people and hard to manage.
There was a problem hiding this comment.
I'm happy to make the documentation clearer on ImageMagick being required for earlier versions of matplotlib and pillow being fine for recent versions, but due to the matplotlib release being so recent I still don't think it should be deprecated.
There was a problem hiding this comment.
I think it would be ok to tell people to upgrade their matplotlib and install pillow if they want gif support. On the plus side, the actual GIF support is handled by matplotlib so it doesn't really change how much code we have to maintain.
I'm happy to go with Philipp's suggestion as long an issue is filed to remove imagemagick support (and references from the docs) in 1.11.
There was a problem hiding this comment.
On the plus side, the actual GIF support is handled by matplotlib so it doesn't really change how much code we have to maintain.
We do declare which backend it uses for rendering GIFs, maybe we can find a way to fall back to other backends cleanly though.
I'm happy to go with Philipp's suggestion as long an issue is filed to remove imagemagick support (and references from the docs) in 1.11.
That seems sensible, but deprecating something in favor of a replacement that's only been around 2-3 weeks just seems too quick.
|
I'll merge now with the understanding that Philipp will file an issue to remove the references to imagemagick when the time is right (I agree it is probably too early to do that now). |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
During the update to the renderer code to make it work in JupyterLab a regression was introduced to rendering animation output using matplotlib (e.g. gif, mp4 and webm output). This PR fixes the regression.
Additionally this switches to the pillow animation writer for matplotlib>=2.2.0, which provides an easy way to generate gifs on windows.