Added line styling keyword arguments to plot outline functions

[JosephHogg]

Added the option to provide 'edgecolors' and 'linewidth' keywords to iris.plot and iris.quicklpot outline functions.

[rhattersley]

👍 for the concept ... I've hacked this together several times but never got round to polishing and submitting a PR.

There's an inconsistency in the quickplot vs. plot interfaces that needs sorting. The quickplot function takes *args, **kwargs and passes them to the plot function, but the plot function only accepts edgecolors and linewidth. Given all the other main plot functions take *args, **kwargs this looks to be pattern to follow.

[JosephHogg]

My thinking was that iris.plot outline is an odd one out, in that it doesn't wrap another function in the same way that iris.plot pcolor wraps pyplot's pcolor function. I figured the quickplot outline function is a wrapper for iris.plot outline so it needn't handle all of the kwargs, but iris.plot outline uses pyplot's pcolormesh to draw the outline of a grid which is more specific functionality than pcolormesh provides.

Essentially, my concern was that if we were to use the *args, **kwargs pattern a user would be able to specify the facecolor kwarg and I wasn't sure if that was appropriate for an outline function.

Could you explain why you think the interfaces in plot and quickplot should be the same?

[ajdawson]

I think you have a good point @Jozhogg. Allowing arbitrary arguments to be passed through to iris.plot.outline isn't sensible as it requires a specific combination of arguments supplied to _draw_2d_from_bounds in order to do its job.

I suggest perhaps the iris.quickplot.outline function is modified to allow only the argument list allowed to iris.plot.outline which will provide the desirable consistency that @rhattersley mentioned.

However, since we are restriciting arguments to iris.plot.outline I'd suggest we make sure they are named something relevant, so instead of edgecolors how about just color? This makes more sense. The function only offers to draw outlines, so what is the edge of an outline? It doesn't really have one., but it does have a color... Also, is there scope for including linestyle too?

[JosephHogg]

Thanks for the help @ajdawson and @rhattersley. I've made the functions in iris.plot and iris.quickplot consistent and changed the keyword argument naming as was suggested.

Unfortunately, pcolormesh will only draw solid lines and so the linestyle argument would not be so simple to add.

[ajdawson]

Unfortunately, pcolormesh will only draw solid lines and so the linestyle argument would not be so simple to add.

No problem, I didn't check before I suggested it.

[rhattersley]

The API makes much more sense now - thanks both of you.

[rhattersley]

This docstring needs updating.

[JosephHogg]

Is this documentation OK? I was a little unsure what guidelines to follow.

[rhattersley]

Is this documentation OK? I was a little unsure what guidelines to follow.

At a glance it looks fine - thanks. The docstrings aren't completely consistent in Iris so it's fair enough to be unsure! I'll build the docs and double-check how they appear...

[ajdawson]

I think normally the part after the colon would try and describe the type of the argument and the line underneath would describe what the argument is for.

The actual types of these arguments can be one of many, for example matplotlib color definitions can be strings with a predefined letter indicating a color, strings containing a hexadecimal RGB value, or a tuple of RGB values etc. You could describe the type of the color argument as "any matplotlib color` and the linestyle as "any matplotlib line style", in fact the matplotlib docs do this.

[rhattersley]

I think normally the part after the colon would try and describe the type of the argument and the line underneath would describe what the argument is for.

👍 ... I didn't push that because I was trying to keep my attention-to-detail habit under control, but maybe I should have so @ajdawson doesn't have to play the bad cop. Sorry @ajdawson 😒

[ajdawson]

No problem @rhattersley! Perhaps it is I who needs to get an attention-to-detail habit under control...

[JosephHogg]

It's OK - I always knew the day would come when I would have to start learning to properly document my code! How does attempt number 2 look?

[rhattersley]

This combines the type of the argument with the meaning of the argument. The preferred form these days is more like:

    * color: None or mpl color
        The color of the cell outlines. If None, the rc setting (which one???) is used by default.

    * linewidth: None or number
        The width of the lines showing the cell outlines. If None, the default width in lines.linewidth(???) in matplotlibrc is used.

NB. This could do with a more consistent description of the behaviour when None. (Or we just ignore it... 😑 )

[JosephHogg]

Was able to confirm the behaviour with None. Thanks for the help @rhattersley

[rhattersley]

The content looks good now, thanks. 👍

Unless @ajdawson's got any more tweaks to suggest then this just needs squashing into a single commit. If you need help with that I'm happy to do it remotely, but it'd probably be easiest to grab someone from the team who's actually in the office.

[ajdawson]

I do have one further item to discuss, sorry!

I think you've actually changed the default behaviour of the function by setting the default value of the linewidth argument to 1. Previously this argument was not used to the internal call to _draw_2d_from_bounds which I think would yield the matplotlibrc default, but now you set it to 1 which means you are overriding the matplotlibrc value. For example, if a user had set their default linewidth to 2 in their matplotlibrc then previously their outlines would be width 2 by default, and now they will be width 1. Do you agree @rhattersley?

For color there is no issue because the previous version explicitly set the colour to black.

[rhattersley]

Do you agree @rhattersley?

Completely! Eagle-:eyes: @ajdawson strikes again! 👍

[JosephHogg]

Good spot! I'll go ahead address that and ask somebody at the office about squashing commits. Thanks both.

[rhattersley]

Thanks for addressing @ajdawson's issue and squashing your commits. This looks to be ready to 🚢 (although I said that before!) ... 👍

[ajdawson]

👍

[rhattersley]

Well done @Jozhogg 🎉