Repository navigation
Reuse png metadata handling of imsave() in FigureCanvasAgg.print_png(). - #15435
QuLogic merged 4 commits into
Conversation
Sorry, something went wrong.
|
Now also includes the warning on metadata/pnginfo collision suggested in #15434 (comment). This effectively requires moving the generation of the Software tag to imsave (as we otherwise don't know whether the tag was autogenerated or provided by the user). |
Sorry, something went wrong.
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?
Sorry, something went wrong.
|
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). |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Would this be fixed by merging 3.2.x into master? If not this needs a re-base. |
Sorry, something went wrong.
|
rebased |
Sorry, something went wrong.
| # semantics of duplicate keys in pnginfo is unclear. | ||
| if "pnginfo" in pil_kwargs: | ||
| if metadata: | ||
| cbook._warn_external("'metadata' is overridden by the " |
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.
Sorry, something went wrong.
There was a problem hiding this comment.
I left this as is but can raise too, just pick one.
Sorry, something went wrong.
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.
This avoids duplicating the conversion of metadata to PngInfo and
revealed a bug in the priority between metadata and pil_kwargs in
imsave(). (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 RGBA
uint8 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