FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

In imsave, let pnginfo have precedence over metadata. by anntzer · Pull Request #15434 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

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

Merged
tacaswell merged 2 commits into
matplotlib:v3.2.xfrom
anntzer:imsave-priority
Oct 19, 2019
ConversationCommits (2)ChecksFiles changed
Merged

In imsave, let pnginfo have precedence over metadata.#15434
tacaswell merged 2 commits into
matplotlib:v3.2.xfrom
anntzer:imsave-priority

Conversation

anntzer commented Oct 17, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

# 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

In imsave, let pnginfo have precedence over metadata.
e9891e2 
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 added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Oct 17, 2019
anntzer added this to the v3.2.0 milestone Oct 17, 2019
anntzer mentioned this pull request Oct 17, 2019
Merged
6 tasks

timhoffm commented Oct 17, 2019

Copy link
Copy Markdown
Member

Is #13092 really the relevant issue here?

timhoffm reviewed Oct 17, 2019
View reviewed changes
Comment thread lib/matplotlib/image.py Outdated
"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):

timhoffm Oct 17, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

anntzer Oct 17, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

timhoffm Oct 17, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

anntzer Oct 17, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

sure, added a warning in both places.

anntzer commented Oct 17, 2019

Copy link
Copy Markdown
Contributor Author

edited: the correct original PR was #13902.

tacaswell merged commit d687c21 into matplotlib:v3.2.x Oct 19, 2019

tacaswell commented Oct 19, 2019

Copy link
Copy Markdown
Member

The doc issue was fixed by #15449 .

anntzer deleted the imsave-priority branch October 20, 2019 08:19
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

timhoffm timhoffm approved these changes

Assignees

No one assigned

Labels

Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.

Projects

None yet

Milestone

v3.2.0

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL