-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Switch to requiring Pillow rather than having our own png wrapper? #15165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
The idea seems reasonable. Is the performance at all dependent on the complexity of the plots, or the size of the pngs? |
I don't have the numbers yet, but can set up more benchmarking. |
👍 if this is going to make both the build process simpler and get us a factor of 2 in writing. |
in reading, not in writing. |
oh, derp, I apparently can not read, but even a small speed up, in conjunction with the easier install is still a win. |
Right now this is stuck on a few upstream issues (python-pillow/Pillow#3041, imageio/imageio#475); may end up needing to vendor some code from imageio with some more fixes to fix that. On the other hand the (unpublished) draft patch I have does indeed make building on Windows much easier. |
It looks like Pillow's png handling (which is hand-written https://github.com/python-pillow/Pillow/blob/master/src/PIL/PngImagePlugin.py, not based on libpng) is faster than matplotlib's, both for writing and for reading:
yields, for me:
#15160 reminded me that it's not always trivial to get C-level dependencies set up before building (though that specific issue is about freetype). We already vendor agg and qhull, and have an option to download freetype (via local_freetype/MPLLOCALFREETYPE), so the sole remaining dependency that always needs to be provided by the builder is libpng.
If we just depended (as an install_requires) on Pillow for png handling instead, we would be able to drop that dependency (together with a bunch of C code). Pillow has wheels for all three major OSes, supports PyPy, is actively developed, and has been around for a very long time.
Thoughts on making the switch?
The text was updated successfully, but these errors were encountered: