Reuse png metadata handling of imsave() in FigureCanvasAgg.print_png(). - #15435
Conversation
timhoffm
left a comment
timhoffm
left a comment
There was a problem hiding this comment.
This is a rather big change just for removing the duplication of metadata and pil_kwargs merging. In particular, replacing a simple Image.save() with the more general mpl.image.imsave()feels a bit risky and obscures the actual quite simple call (I didn't easily manage to follow if imsave() with the given parameters is equivalent to the original code).
Extracting a _merge_metadata_pil_kwargs() function seems to be a simpler solution. - Or did I miss any other important stuff that imsave() is now doing?
|
The calls are equivalent, although I readily admit that it's not obvious to follow them (the relevant point is at matplotlib/lib/matplotlib/cm.py Line 221 in eb8327d matplotlib/lib/matplotlib/cm.py Line 236 in eb8327d As suggested above one of the reasons to move the pil_kwargs handling logic in imsave() is to let mplcairo also take advantage of it (well, if I need to call private APIs in mplcairo I can do it too :) but I'd rather not). |
anntzer
added
the
Release critical
|
Not urgent by any means, but does need to get into 3.3 to prevent a (subtle) regression against 3.2, for which the same issue was fixed in #15434, so tagging as release critical. |
|
Would this be fixed by merging 3.2.x into master? If not this needs a re-base. |
|
rebased |
There was a problem hiding this comment.
I could also see a case for raising, but warning seems like a step in the right direction.
There was a problem hiding this comment.
I left this as is but can raise too, just pick one.
This avoids duplicating the conversion of metadata to PngInfo and revealed a bug in the priority between `metadata` and `pil_kwargs` in imsave(). Note that because `np.asarray(self.buffer_rgba())` is already a RGBA uint8 array, there is no colormapping step happening in imsave(). Ideally mplcairo should also be able to use imsave() for saving to png.
4 participants
This avoids duplicating the conversion of metadata to PngInfo and
revealed a bug in the priority between
metadataandpil_kwargsinimsave(). (That bug is also fixed for 3.2 in #15434; this PR effectively
tests that.)
Note that because
np.asarray(self.buffer_rgba())is already a RGBAuint8 array, there is no colormapping step happening in imsave().
Ideally mplcairo should also be able to use imsave() for saving to png.
PR Summary
PR Checklist