-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
replace cairo.format with savefig.format in rcParams #907
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
Conversation
lib/matplotlib/rcsetup.py
Outdated
There was a problem hiding this comment.
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'
|
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. |
|
Note also the existence of What I think should happen here is that we deprecate |
|
@pelson, OK, file format in rcParams is checked at runtime now. I left @mdboom, I think I've removed support for the |
|
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. |
lib/matplotlib/backend_bases.py
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
That was a little tricky, but I think I've implemented everything that @mdboom suggested. The deprecation machinery was already there in Sorry about the annoying whitespace changes in |
lib/matplotlib/rcsetup.py
Outdated
There was a problem hiding this comment.
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.
|
@mspacek: |
|
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? |
…ackends replace some stray tabs with spaces
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
… until runtime add docstring
|
I think the rcParams['savefig.format'] key is now unrestricted. Seems to properly report unsupported file types when calling Also, I hope I've rebased properly against origin/master. Git stresses me out :) |
|
This looks good to me. Merging. |
replace cairo.format with savefig.format in rcParams
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) > 1clause in__init__.pynow raises aValueError('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?