Skip to content

Conversation

@mspacek
Copy link
Contributor

@mspacek mspacek commented May 28, 2012

I stumbled across this post on the mailing list from a while ago about creating an rcParams field that controls the default savefig filetype:

http://www.mail-archive.com/matplotlib-users@lists.sourceforge.net/msg23382.html

It was suggested that cairo.format be deprecated and replaced with a single rcParam that applies to all (relevant) backends. So here's my stab at it. savefig.format is a new key that's loaded by the base backend, and only overridden in backends that didn't default to .png as their hardcoded format (specifically, the PS, PDF, SVG, and EMF backends). Seems pretty straightforward, but I've only really tested this on the Qt4Agg backend, because that's all I use.

I only did this because I wanted to change the default savefig format from .png to .pdf in my matplotlibrc file, and now I can.

This also inadvertently replaced a few stray tabs with spaces in a couple of rc files.

By the way, the if len(be_parts) > 1 clause in __init__.py now raises a ValueError('FIXME: Not sure what to do here'), because I wasn't exactly sure what it was doing wrt cairo.format. But, I'm guessing the whole clause can be deleted. Am I right?

Copy link
Member

Choose a reason for hiding this comment

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

The problem with this, as far as I can see, is that you cannot guarantee what filetypes are valid until you know what backend is being used, and you cannot know what backend is being used until runtime. Therefore, for many of the backends, it would make sense for get_default_filetype to do something like:

def get_default_filetype(self):
    default = rcParams['savefig.format']
    if default in self.get_supported_filetypes().keys():
        return default
    else:
        return 'png'

@mdboom
Copy link
Member

mdboom commented May 29, 2012

I agree with @pelson's comments.

Some history here: the Cairo backend was initially the only backend that supported multiple output types, hence it has a special way of doing it. Much later I added support for saving PDF, PS, SVG etc. directly from any of the GUI backends without having to tinker with switching backends etc. I think the next step of removing Cairo's special way of doing things is probably a good idea. I think it's fine to make this kind of change on master, but it should have a CHANGELOG entry outlining the backward incompatibility.

@mdboom
Copy link
Member

mdboom commented May 29, 2012

Note also the existence of savefig.extension which determines the default filetype when using savefig, but not from the GUI dialog. I think savefig.extension and savefig.format should be merged into a single value -- better than having two values that do almost the save thing. (Or is there any benefit to setting it differently for GUI save dialogs and for calling savefig?)

What I think should happen here is that we deprecate savefig.extension (savefig.format is a better name). So setting savefig.extension raises a DeprecationWarning. If savefig.format is defined, it overrides the value of savefig.extension. If savefig.format is not defined, savefig.extension is used. In the next major release, we go back and remove all references to savefig.extension. All of that magic can be crammed into get_default_filetype. In addition, Figure.savefig should be updated to use get_default_filetype rather than accessing rcParams['savefig.extension'] directly.

@mspacek
Copy link
Contributor Author

mspacek commented May 31, 2012

@pelson, OK, file format in rcParams is checked at runtime now. I left validate_savefig_format() as is, not sure if that's correct. I added docstrings to get_default_filetype() and get_supported_filetypes(), but I'm not really sure what get_supported_filetypes_grouped does.

@mdboom, I think I've removed support for the cairo.<format> style of backend specificiation. I suppose CHANGELOG entries are added only right before merging? I'll see if I can combine savefig.extension with savefig.format

@mdboom
Copy link
Member

mdboom commented May 31, 2012

Looks good. You can feel free to add a CHANGELOG entry at any time, as long it tracks the current status of the pull request.

Copy link
Member

Choose a reason for hiding this comment

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

I think this method's docstring should mention the rcParam that it uses (i.e. explicitly mention savefig.format).

Also, mention that the returning extension does not include the period, and change the If invalid, use .png to If invalid, use "png".

Finally, more at @mdboom, is this the behaviour that we actually want? For instance, if a user specifies "raw" to be the default filetype, shouldn't that be what they get, independent of which backend they are using for their GUI.

Copy link
Member

Choose a reason for hiding this comment

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

@pelson: Any of the GUI backends should be able to save to any of the formats (the same doesn't hold true for non-GUI backends), so as per my other comment below, I think the savefig.format parameter should be unrestricted as to what it takes, and then let the backend decide whether it's valid at save time.

@mspacek
Copy link
Contributor Author

mspacek commented Jun 1, 2012

That was a little tricky, but I think I've implemented everything that @mdboom suggested. The deprecation machinery was already there in rcsetup.py and __init__.py, so that's where the magic happens. Haven't examined that machinery much, but it seems to do what was desired.

Sorry about the annoying whitespace changes in api_changes.rst, but my editor replaces tabs with spaces by default, and that's probably something that should be done anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Should we limit ourselves here? As you pointed out, there's no way to know what the complete selection is until a backend has been selected, but it could include other things, such as emf, gif, jpg depending on the backend. I think it best to just make this "any string" and let it be verified by the backend upon saving.

@mdboom
Copy link
Member

mdboom commented Jun 1, 2012

@mspacek: get_supported_filetypes_grouped returns a dictionary where each key is a logical filetype and the values are a list of extensions for that filetype. For example, {"Joint Photographic Experts Group": ['jpg', 'jpeg']}. Some GUI frameworks, eg. wx ang qt4, allow this kind of presentation in their save dialog. Others, eg. gtk, don't. So maybe the docstring could be:

Returns a dictionary of supported filetypes where the keys are a file type name, such as 'Joint Photographic Experts Group', and the values are a list of filename extensions used for that filetype, such as ['jpg', 'jpeg'].

@mdboom
Copy link
Member

mdboom commented Jun 1, 2012

Other than my comment about making the rcParam unrestricted in what it takes, this is really shaping up. Thanks. Would you also mind doing a rebase against current master so it will be automatically mergeable?

mspacek added 6 commits June 2, 2012 18:51
remove support for 'cairo.pdf' type of backend specification
remove all mention of cairo.<format>
remove unnecessary check for supported formats in get_default_filetype, rely on check in _get_print_method instead
for uniformity, use get_supported_filetypes instead of directly accessing self.filetypes
@mspacek
Copy link
Contributor Author

mspacek commented Jun 3, 2012

I think the rcParams['savefig.format'] key is now unrestricted. Seems to properly report unsupported file types when calling savefig() or when saving from within the GUI window's save dialog, although I've noticed that for some reason my ipython seems to swallow the popup error dialog box when the format is unsupported. This doesn't happen when I'm running ipython qtconsole or just plain python. Not sure if that's a worry.

Also, I hope I've rebased properly against origin/master. Git stresses me out :)

@mdboom
Copy link
Member

mdboom commented Jun 6, 2012

This looks good to me. Merging.

mdboom added a commit that referenced this pull request Jun 6, 2012
replace cairo.format with savefig.format in rcParams
@mdboom mdboom merged commit d17dbff into matplotlib:master Jun 6, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants