| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
Sorry, something went wrong.
|
It is nice how simple this patch is to pick up a new format! Could you add a test that saves an image to ByteIO buffer? That will be enough to exercise the code that you added. Given that this is using the Agg backend we definitely do not need to run it against the full test suite as we would then just be testing pillow. It looks like print_tiff is covered, maybe find that test and adapt / parameterize it? |
Sorry, something went wrong.
|
@tacaswell Yes, I will add a test using ByteIO buffer. |
Sorry, something went wrong.
| print_tiff = print_tif | ||
|
|
||
| @_check_savefig_extra_args | ||
| def print_webp(self, filename_or_obj, *, pil_kwargs=None): |
There was a problem hiding this comment.
Are all these pillow save formats the same? If so, I wonder if we just want a generic print_pillow(format=pillow_format) rather than adding these one-by-one?
Sorry, something went wrong.
There was a problem hiding this comment.
As far as I can tell, it is the same for all the fully supported formats- BMP, DDS, DIB, EPS, GIF, ICNS, ICO, IM, JPEG, JPEG 2000, MSP, PCX, PNG, PPM, SGI, SPIDER, TGA, TIFF, WebP, XBM.
I think adding a generic print_pillow does make a lot of sense especially if we want to support all of these formats. The tests however will have to be created for each format, as they all have their specific options that need to be tested.
Sorry, something went wrong.
There was a problem hiding this comment.
Thats the approach I would take, but maybe its more complex than that...
Sorry, something went wrong.
There was a problem hiding this comment.
Maybe we should leave it for a different PR then?
Sorry, something went wrong.
There was a problem hiding this comment.
I guess that depends on whether we think webp is used enough to merit a standalone method.
Sorry, something went wrong.
There was a problem hiding this comment.
I suspect the way we would have to do this in general would be some mild-meta-programming as
matplotlib/lib/matplotlib/backend_bases.py
Line 2250 in a698063
matplotlib/lib/matplotlib/backend_bases.py
Lines 2236 to 2246 in a698063
However, we can write ourselves a print function factory function and in backend_agg.py do something like:
for fmt in LIST_OF_PILLOW_FORMATS:
setattr(BackendAgg, f'print_{fmt}', pillow_printer_factor(fmt)
Sorry, something went wrong.
|
@kinshukdua Would you be willing to rebase this PR? We prefer to not merge the default branch back into feature branches. |
Sorry, something went wrong.
|
@tacaswell I've rebased the PR, I'm new to git so please let me know if I need to do something else. |
Sorry, something went wrong.
There was a problem hiding this comment.
I think we need to decide whether we want a new webp method, a general "pass to pillow" method or a factory. If in the end, we don't want a new webp method, I dont think we can add this.
I'm not against a webp method if it really is something many folks want, but it seems obscure to me, and maybe just a general method to pillow takes care of the obscure use cases.
I'm blocking until we can sort this out, but if another core dev wants to support a standalone webp method, feel free to dismiss my review.
Sorry, something went wrong.
|
@tacaswell said that this is what we want, which is fine with me. Thanks for your patience! |
Sorry, something went wrong.
There was a problem hiding this comment.
Please consider clarifying the situation wrt. dpi. Anyone can dismiss this.
Sorry, something went wrong.
|
Thanks, anyone can merge after CI passes. Congrats on getting your first PR in :) |
Sorry, something went wrong.
|
Thanks @kinshukdua! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again. |
Sorry, something went wrong.
|
Thanks! It was really great contributing back to such an important open source project, I hope to continue contributing. |
Sorry, something went wrong.
ENH: Add support to save images in WebP format
| Back | FazBrowse Home | New Git URL |
PR Summary
Fixes #21162
This adds support to save images in WebP format.
This works fine without any problem since Pillow supports WebP and the code is analogous to the support for tiff images added in PR #15193.
PR Checklist