NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.3k

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

tacaswell merged 2 commits intomatplotlib:v3.2.xfromanntzer:imsave-priority

Oct 19, 2019

Merged

In imsave, let pnginfo have precedence over metadata.#15434

tacaswell merged 2 commits intomatplotlib:v3.2.xfromanntzer:imsave-priority

Oct 19, 2019

Conversation

Copy link

Contributor

anntzer commentedOct 17, 2019•
edited
Loading

This is consistent with the documented behavior ("If the 'pnginfo' key
is present, it completely overridesmetadata, 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 inc9f292f

	# Only use the metadata kwarg if pnginfo is not set, because the
	# semantics of duplicate keys in pnginfo is unclear.
	if"pnginfo"notinpil_kwargs:

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

Has Pytest style unit tests
Code isFlake 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

In imsave, let pnginfo have precedence over metadata.

e9891e2

This is consistent with the documented behavior ("If the 'pnginfo' keyis present, it completely overrides *metadata*, including the default'Software' key.") and also with the behavior ofFigureCanvasAgg.print_png.

anntzer added the Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label

Oct 17, 2019

anntzer added this to thev3.2.0 milestone

Oct 17, 2019

anntzer mentioned this pull request

Oct 17, 2019

Reuse png metadata handling of imsave() in FigureCanvasAgg.print_png().#15435

Merged

6 tasks

Copy link

Member

timhoffm commentedOct 17, 2019

Is#13092 really the relevant issue here?

timhoffm reviewed

Oct 17, 2019

View reviewed changes

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):

Copy link

Member

timhoffmOct 17, 2019•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Silently ignoringmetadata ifpnginfo 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?

Copy link

ContributorAuthor

anntzerOct 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

print_png (used bysavefig("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 ???").

Copy link

Member

timhoffmOct 17, 2019

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.