Skip to content

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

Merged
merged 2 commits into from
Oct 19, 2019

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Oct 17, 2019

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

# Only use the metadata kwarg if pnginfo is not set, because the
# semantics of duplicate keys in pnginfo is unclear.
if "pnginfo" not in pil_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 is Flake 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

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.
@anntzer anntzer added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Oct 17, 2019
@anntzer anntzer added this to the v3.2.0 milestone Oct 17, 2019
@timhoffm
Copy link
Member

Is #13092 really the relevant issue here?

@@ -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

@timhoffm timhoffm Oct 17, 2019

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?

Copy link
Contributor Author

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 ???").

Copy link
Member

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.

Copy link
Contributor Author

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.

@anntzer
Copy link
Contributor Author

anntzer commented Oct 17, 2019

edited: the correct original PR was #13902.

@tacaswell tacaswell merged commit d687c21 into matplotlib:v3.2.x Oct 19, 2019
@tacaswell
Copy link
Member

The doc issue was fixed by #15449 .

@anntzer anntzer deleted the imsave-priority branch October 20, 2019 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants