Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Reuse png metadata handling of imsave() in FigureCanvasAgg.print_png().#15435
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is a rather big change just for removing the duplication of metadata and pil_kwargs merging. In particular, replacing a simpleImage.save() with the more generalmpl.image.imsave()feels a bit risky and obscures the actual quite simple call (I didn't easily manage to follow ifimsave() 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 thatimsave() 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 ineb8327d
matplotlib/lib/matplotlib/cm.py Line 236 ineb8327d
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). |
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 |
Uh oh!
There was an error while loading.Please reload this page.
| # 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I left this as is but can raise too, just pick one.
This avoids duplicating the conversion of metadata to PngInfo andrevealed a bug in the priority between `metadata` and `pil_kwargs` inimsave().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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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