Bad kwargs to savefig #1112

Closed
wants to merge 2 commits into
from

Projects

None yet

2 participants

@pelson
Member
pelson commented Aug 20, 2012

This is an attempt to resolve #192.
I would like it to get some visibility before the 1.2.x cutoff, but in reality I do not expect it to make it into the freeze.

@pelson pelson commented on the diff Aug 20, 2012
lib/matplotlib/figure.py
@@ -1218,10 +1216,10 @@ def savefig(self, *args, **kwargs):
patch.set_facecolor('none')
patch.set_edgecolor('none')
else:
- kwargs.setdefault('facecolor', rcParams['savefig.facecolor'])
- kwargs.setdefault('edgecolor', rcParams['savefig.edgecolor'])
+ facecolor = facecolor or rcParams['savefig.facecolor']
+ edgecolor = edgecolor or rcParams['savefig.edgecolor']
@pelson
pelson Aug 20, 2012 Member

BUG: I intended to use the edgecolor & facecolor and then reset them at the savefig level so that the backends don't have to do this. Unfortunately this is half baked at the moment (writing a test for this might be interesting).

@pelson pelson commented on the diff Aug 20, 2012
lib/matplotlib/backends/backend_emf.py
width, height = self.figure.get_size_inches()
- renderer = RendererEMF(filename,width,height,dpi)
+ renderer = RendererEMF(filename, width, height, self.figure.dpi)
@pelson
pelson Aug 20, 2012 Member

This doesn't look equivalent. I need to re-address this.

@pelson pelson commented on the diff Aug 20, 2012
lib/matplotlib/backends/backend_macosx.py
# original dpi. Pick it up from the renderer.
- dpi = kwargs['dpi']
- old_dpi = self.figure.dpi
+ dpi = self.figure.dpi
@pelson
pelson Aug 20, 2012 Member

Again, doesn't appear equivalent.

@pelson pelson commented on the diff Aug 20, 2012
lib/matplotlib/backends/backend_pdf.py
@@ -2259,20 +2259,20 @@ def draw(self):
def get_default_filetype(self):
return 'pdf'
- def print_pdf(self, filename, **kwargs):
- image_dpi = kwargs.get('dpi', 72) # dpi to use for images
+ def print_pdf(self, filename, bbox_inches_restore=None):
+ image_dpi = self.figure.dpi # dpi to use for images
@pelson
pelson Aug 20, 2012 Member

Ah, ok. I wrote these changes a little while back so its like I'm coming to it new!

Again this is half baked, but the intention was to have the savefig manage the figures dpi rather than passing it through, that way, the savefig method can re-set it after calling the backend.

@pelson
Member
pelson commented Aug 20, 2012

This PR is not for merging at this stage. There are several outstanding issues which would need addressing.
More importantly at this stage is to get some feedback on the way the PR is going. Is it better to be strict about the keywords which can be passed? It will result in more exceptions, but fewer cases where users get unexpected/no behaviour from certain keywords.

@WeatherGod
Member

I definitely support the end-goal here, but I am not entirely sure if it ever would be feasible to achieve. I should have some time today to look over this and comment.

@pelson
Member
pelson commented Nov 29, 2012

Personally, I find this PR an improvement, but it does come with some costs (having to explicitly type out all of the keywords that are supported in a function). I would love to get some feedback on this, so that ultimately, I can either close this PR, or update it into a form that we can merge.

Please don't review it as if it were about to be merged - I suspect I will have to start from scratch on this once I have some general feedback and an agreement that this is, in principle, something we want to do.

Cheers,

@pelson
Member
pelson commented Feb 18, 2013

I'm not sure this is something we want to move towards at this stage - it improves error messages, but at the cost of freedom to pass through arbitrary keywords. If it were something we wanted to do then it would be a great task for a sprint at a conference etc.

@pelson pelson closed this Feb 18, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment