bug: Figure.savefig(fname="...") not working

[megies]

Due to a mismatch in kwarg names Figure.savefig(fname="...") is raising an exception (i.e. specifying output filename as kwarg as opposed to first (and only) arg. I think this bug was introduced with 8cdc0da.

Code to reproduce:

import matplotlib.pyplot as plt

plt.switch_backend("AGG")

fig = plt.figure()
plt.plot(range(5))
fig.savefig("/tmp/savefig1.png")  # works
fig.savefig(fname="/tmp/savefig2.png")  # does not work, raises

Traceback (most recent call last):
  File "/tmp/savefig_bug.py", line 8, in <module>
    fig.savefig(fname="/tmp/savefig2.png")
  File "/home/megies/git/matplotlib/lib/matplotlib/figure.py", line 1539, in savefig
    self.canvas.print_figure(*args, **kwargs)
TypeError: print_figure() takes at least 2 arguments (4 given)

The attached commit fixes the problem.

[tacaswell]

I don't quite understand how that commit (or the PR it is in #907) caused this bug. Looking back it is not clear to me this ever worked.

Can you add a check that both fname and filename are not passed in?

[megies]

Looking back it is not clear to me this ever worked.

Hmm, yeah, not sure either, actually. We had one instance of this in obspy/obspy (see above linked issue) and it was reported to us today. I'm pretty sure I would have come across it had it been in matplotlib some years ago, but I'm not sure and I guess it doesn't make a difference, just needs fixing now.

Can you add a check that both fname and filename are not passed in?

I'm not sure about this.. savefig() clearly states its args/kwargs in the docstring and there's no mention of filename so it should not be passed in there via kwargs. I would rather think about changing the overly general call syntax from def savefig(self, *args, **kwargs): to what is specified in the docstring (don't get me wrong, I know this cleaning up is always scary because you never know which codes it might break..)

[mdboom]

I milestoned too quickly on this one based on the reported introduction in 8cdc0da. I think we could safely do this later given where we are in the 1.5.0 cycle.

I fear this is one of those dark things where the docs has just been wrong forever. It's been referred to as fname in the savefig docs since at least 2006, and filename in print_figure since around the same time.

[megies]

I fear this is one of those dark things where the docs has just been wrong forever.

Yeah, I know what you mean. One more reason to get rid of legacy def ...(self, *args, **kwargs).
Feel free to close and handle this differently in any way deemed appropriate.

[tacaswell]

Part of the problem here is that we are doing our own arg/kwarg parsing
here. A better fix might be to tack kwargs['fname'] on to the front of
args, but that also had it's own issues.

The kwargs are passed through so passing filename currently works. If we
are going to and the feature of passing fname via kwarg, we need to do a
bit more validation.

On Mon, Oct 26, 2015, 17:55 Michael Droettboom notifications@github.com
wrote:

I milestoned too quickly on this one based on the reported introduction in
8cdc0da
8cdc0da.
I think we could safely do this later given where we are in the 1.5.0 cycle.

I fear this is one of those dark things where the docs has just been wrong
forever. It's been referred to as fname in the savefig docs since at least
2006, and filename in print_figure since around the same time.

—
Reply to this email directly or view it on GitHub
#5323 (comment)
.

[tacaswell]

punted to next bug-fix release.

[phobson]

This seems good, but it'd be good to include a test to show that this works.

[jklymak]

I don't think this PR is needed anymore with 3.0; fname is a positional argument, and our signature strictly enforces it, so unless I'm misunderstanding fname="" is not allowed. Feel free to re-open if I'm incorrect.

def savefig(self, fname, *, frameon=None, transparent=None, **kwargs):