-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
In imsave, let pnginfo have precedence over metadata. #15434
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
Conversation
This is consistent with the documented behavior ("If the 'pnginfo' key is present, it completely overrides *metadata*, including the default 'Software' key.") and also with the behavior of FigureCanvasAgg.print_png.
Is #13092 really the relevant issue here? |
lib/matplotlib/image.py
Outdated
@@ -1539,7 +1539,8 @@ def imsave(fname, arr, vmin=None, vmax=None, cmap=None, format=None, | |||
pil_shape = (rgba.shape[1], rgba.shape[0]) | |||
image = Image.frombuffer( | |||
"RGBA", pil_shape, rgba, "raw", "RGBA", 0, 1) | |||
if format == "png" and metadata is not None: | |||
if (format == "png" | |||
and metadata is not None and "pnginfo" not in pil_kwargs): |
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.
Silently ignoring metadata
if pnginfo
is present does not feel right. Can we raise or warn in that case, or are there subsets of metadata that may still be needed?
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.
print_png (used by savefig("foo.png")
has had this behavior ever since pil_kwargs is supported (which is 3.1), and no one has complained about it so far (it probably hasn't been too much used yet in the wild, though); I'd rather keep both places in sync.
Also a verb is missing in your sentence ("Can we ???").
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.
Fixed the missing verb. I'd be happy to warn in both places. Raising would now be a breaking change.
I wouldn't expect too much complaint. It's not widely used, it will only happen if you provide both parameters. It might actually have affect users without them realizing it.
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.
sure, added a warning in both places.
edited: the correct original PR was #13902. |
e7e6343
to
9e9c5d1
Compare
The doc issue was fixed by #15449 . |
This is consistent with the documented behavior ("If the 'pnginfo' key
is present, it completely overrides metadata, including the default
'Software' key.") and also with the behavior of
FigureCanvasAgg.print_png, cf
matplotlib/lib/matplotlib/backends/backend_agg.py
Lines 522 to 524 in c9f292f
The feature came in in #13902 which is not in any released version so far, so it would be nice if this was fixed before the final 3.2 release. I will prepare a separate PR for master.
PR Summary
PR Checklist