Add figsize, size, and aspect arguments to plotting methods #1168
Conversation
|
After having another look, |
|
OK, this should be ready for review now |
| vmin=cmap_params['vmin'], | ||
| vmax=cmap_params['vmax'], | ||
| **kwargs) | ||
| if 'imshow' == plotfunc.__name__ and isinstance(aspect, basestring): |
There was a problem hiding this comment.
I think it would be better to avoid this special case, and simply not pass aspect on to imshow. Xarray imshow is already overloaded to display in data coordinates (not pixels) so I think this is pretty reasonable.
There was a problem hiding this comment.
OK. I still have to catch this possibility though, otherwise the error message is not very informative
There was a problem hiding this comment.
Can you clarify -- what happens if you try write array.plot.imshow(aspect=2)?
I think this should probably result in a plot with the same figure size as array.plot.contourf(aspect=2).
There was a problem hiding this comment.
Yes, sorry. Both methods will raise the same ValueError if called with aspect=2:
ValueError: cannot provide `aspect` argument without `size`
The point of this line is to provide a more informative error message when users try to set aspect='equal', as in this test for example. If you're happy with the message above I can remove this block.
There was a problem hiding this comment.
Oh OK, I was confused about this :). I agree, this makes complete sense.
| # create a dummy figure so sphinx plots everything below normally | ||
| plt.figure() | ||
|
|
||
| This feature also works with :ref:`plotting.faceting`. |
There was a problem hiding this comment.
I think figsize refers to the entire figure when used with faceting, but size only refers to a single panel. We should either make that consistent (both referring to single panels?) or note the difference.
There was a problem hiding this comment.
I think that figsize should always refer to the entire figure, as in matplotlib. I added a note to the docs
|
OK, this should be ready for another round of reviews |
| **kwargs) | ||
| if 'imshow' == plotfunc.__name__ and isinstance(aspect, basestring): | ||
| # forbid usage of mpl strings | ||
| raise ValueError("plt.imshow's `aspect` kwarg is not available " \ |
There was a problem hiding this comment.
Just use implicitly string concatenation here instead of writing \ (you almost never want/need to actually use \)
Extends and finishes #637
I chose to keep seaborn's convention for two reasons:
figsizeis also available, I expect thesizeandaspectkwargs to be less used in non-facetgrid plotsCloses #897