Skip to content

Add description for metadata argument of savefig #11755

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 1 commit into from
Jul 28, 2018

Conversation

timhoffm
Copy link
Member

PR Summary

Fixes #11749


Parameters
----------
filename_or_obj : str or PathLike of file-handle
Copy link
Contributor

Choose a reason for hiding this comment

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

or

metadata : dict, optional
Metadata in the PNG file as key-value pairs of strings.
According to the PNG specification, keys must be shorter than 79
chars. The only supported encodingfor both keywords and values is
Copy link
Contributor

Choose a reason for hiding this comment

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

space

@@ -1979,7 +1979,7 @@ def savefig(self, fname, *, frameon=None, transparent=None, **kwargs):
savefig(fname, dpi=None, facecolor='w', edgecolor='w',
orientation='portrait', papertype=None, format=None,
transparent=False, bbox_inches=None, pad_inches=0.1,
frameon=None)
frameon=None, metadata)
Copy link
Contributor

Choose a reason for hiding this comment

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

metadata={}

Copy link
Member Author

Choose a reason for hiding this comment

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

I've used None instead of {} because there are non-empty defaults depending on the backend.

@timhoffm timhoffm force-pushed the doc-savefig-metadata branch 2 times, most recently from ddbb0f6 to 20fadd7 Compare July 23, 2018 20:24
The file to write to.

metadata : dict, optional
Metadata in the PNG file as key-value pairs of strings.
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the implementation of _png.cpp, looks like this should actually be "of latin-1-encodable strs or of bytes". The relevant chunk:

            if (PyUnicode_Check(meta_key)) {
                PyObject *temp_key = PyUnicode_AsEncodedString(meta_key, "latin_1", "strict");
                if (temp_key != NULL) {
                    text[meta_pos].key = PyBytes_AsString(temp_key);
                }
            } else if (PyBytes_Check(meta_key)) {
                text[meta_pos].key = PyBytes_AsString(meta_key);

@timhoffm timhoffm force-pushed the doc-savefig-metadata branch from 20fadd7 to 52c36af Compare July 23, 2018 21:17
According to the PNG specification, keys must be shorter than 79
chars.

The PNG specification defines some common keywords that may be
Copy link
Member

Choose a reason for hiding this comment

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

This is quite a long list, it might be worth linking to the relevant part of the PNG specification instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the hint. I've added a link to the PNG specification. Nevertheless, I think it's good to have a list of the common keys here so that people don't have to check the external documentation just to get an idea how this is typically used.

@dstansby
Copy link
Member

👍 , looks like it might need a rebase for circleci to work, and I think adding a link to the PNG specs would be good too.

@timhoffm timhoffm force-pushed the doc-savefig-metadata branch 2 times, most recently from 44d996d to 6768d6c Compare July 24, 2018 22:54
@timhoffm
Copy link
Member Author

Rebased.

@dstansby dstansby added this to the v3.0 milestone Jul 24, 2018
@dstansby
Copy link
Member

Anyone feel free to merge if/when CI passes.

will be used.

For more details see the PNG specification:
https://www.w3.org/TR/2003/REC-PNG-20031110/#11keywords
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a huge deal but I've grown fond of out-of-line rst links, e.g. here I'd have written before the list

`The PNG specification`_ defines some common keywords ...

and at the bottom

_The PNG specification: https://...

This way the plain text is just as readable and you get just a link in the HTML, which is normally what you want.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does this work with line lengths? The link definition at the bottom would exceed 79 chars.

Copy link
Contributor

Choose a reason for hiding this comment

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

_The PNG specification: \
   https://...

should also work (as long as the backslash stays an escape -- don't turn the docstring into a raw string!)

@dstansby
Copy link
Member

Looks like the doc build is failing because of

Warning, treated as error:
/home/circleci/project/lib/matplotlib/backends/backend_agg.py:docstring of matplotlib.backends.backend_agg.FigureCanvasAgg.print_png:23:Unexpected indentation.

- 'png' with Agg backend: See the parameter ``metadata`` of
`~.FigureCanvasAgg.print_png`.
- 'pdf' with pdf backend: See the parameter ``metadata`` of
`.PdfPages`.
Copy link
Member

Choose a reason for hiding this comment

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

This needs to specify between matplotlib.backends.backend_pdf.PdfPages and matplotlib.backends.backend_pgf.PdfPages I wasn't sure if you are OK w/ pushing to your PRs - if so next time I'll just push minor changes like this...

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure I understand the differences between pdf and pgf. You are welcome to push to my PRs.

Copy link
Member

Choose a reason for hiding this comment

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

Pgf needs tikz and a latex compiler to produce a PDF using the pgf file. I had no idea that it had a PdfPages method.

Copy link
Member Author

@timhoffm timhoffm Jul 28, 2018

Choose a reason for hiding this comment

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

Ok. I've now linked to backend_pdf.PdfPages, which was my original intention.

@timhoffm timhoffm force-pushed the doc-savefig-metadata branch from 390a508 to 282c635 Compare July 27, 2018 21:49
@timhoffm timhoffm force-pushed the doc-savefig-metadata branch from 282c635 to d6c3885 Compare July 28, 2018 00:55
@jklymak jklymak merged commit e66de9c into matplotlib:master Jul 28, 2018
@timhoffm timhoffm deleted the doc-savefig-metadata branch July 28, 2018 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants