Extend 1D plotting capabilities. Resolves #581.

[ajdawson]

Extended 1D plotting capabilities

This PR changes the 1D plotting interfaces (iris.plot.plot and iris.quickplot.plot) so that they take either one or two positional arguments (like matplotlib.pyploy.plot) which can be either coordinates or cubes. This allows the user to have full control over the resulting plot.

This replaces the much more limited #582 (hat tip to @esc24: you were right, I was wrong... this is a much better idea!)

Summary

iris.plot.plot and iris.quickplot.plot can take up to two positional arguments, which can be either coordinates or cubes. The coords keyword argument has been deprecated in these functions as it is no longer necessary/useful.

• Deprecated coords keyword in iris.plot.plot
• Re-written _draw_1d_from_points to handle one or two cubes or coordinates, done in such a way that y-axis inversion for vertical coords #522 and other future additions can be made with minimal effort.
• Deprecated coords keyword in iris.quickplot.plot.
• New labelling function for quickplot as the existing code could not easily be bent to handle the new feature.
• Removed coords keyword argument from examples that use it for 1D plots.
• Added tests for the new behaviour.

Remaining issues

• Decide what (if anything) should be the title for plots of two coordinates or two cubes when using the quickplot interface.
• Should coord vs. coord maps have axis labels? Currently they do not which is consistent with the other map plotting functions.

[rhattersley]

I only had a quick flick through, but it looks good - thank you. 😀

[ajdawson]

Some very confusing Travis CI results...

Edit: the weird error went away after a rebase (corrupt upload the first time perhaps, I've no idea, Github was behaving badly this afternoon...), the PEP8 error will be fixed by #597, I'll rebase.

[ajdawson]

The tests for this branch pass locally, I still haven't managed to get a working Travis build, last time due to a time-out in the build, although it lists the build as successful.

[ajdawson]

I've knocked together an example http://nbviewer.ipython.org/5916765. It is trying to recreate @esc24's example in #582 using the new plot API.

[rhattersley]

I've knocked together an example http://nbviewer.ipython.org/5916765. It is trying to recreate @esc24's example in #582 using the new plot API.

👏 Very nice indeed. It would make a great addition to the user guide too.

[rhattersley]

I think we still need the old coords argument documented (including a deprecation statement). I guess we don't want it particularly prominent, so perhaps in a .. note:: block?

[ajdawson]

This is tricky, the argument is deprecated, and setting it will do nothing except raise a warning. It is not like a lot of other cases where you see a warning but still get the behaviour you expected... some careful thinking is required.

[rhattersley]

Hmm.. that's a very good point. The "coords" argument is not so much "deprecated" as just "gone"! It really needs to continue to function whilst emitting the warning.

[ajdawson]

Yeah I was coming round to that conclusion myself. I can add some checks in plot to see if a single cube is input along with the coords keyword, and if so construct a new argument list that will make a plot the same as one would expect from the old interface.

[rhattersley]

Resolves #581.

#581 also calls for "some cleverness to automatically detect vertical coordinates" ... which could still be done after this PR is merged. So perhaps we should leave #581 open ... ?

[ajdawson]

This PR satisfies the first part of that statement so I'd prefer to close it and make a separate issue of the "cleverness" part, I think this would make for a simpler issue.

[rhattersley]

This PR satisfies the first part of that statement so I'd prefer to close it and make a separate issue of the "cleverness" part, I think this would make for a simpler issue.

That's fine by me.

[ajdawson]

I've made some changes based on @rhattersley's comments, namely:

• proper deprecation process for the coords keyword argument
• refactor of some of the utility code

[ajdawson]

Good point @rhattersley. I thought about pulling out that inner as_coord function, but since this is going into deprecation code in two separate modules I thought it would be just as easy to copy the 4 lines that do the test into each? I wouldn't do this for code I expected to stay around for long, but since this won't...

How long is a deprecation cycle for iris anyway? Will the code we are talking about will be present in iris 1.5, and then 1.6 too and removed in 1.7? Or removed in 1.6? Or later?

[ajdawson]

@rhattersley I decided to implement a decorator to handle the deprecation of the coords keyword in iris.plot.plot and iris.quickplot.plot. Hopefully this is easier to deal with once the deprecation period is over.

[rhattersley]

Good point @rhattersley. I thought about pulling out that inner as_coord function, but since this is going into deprecation code in two separate modules I thought it would be just as easy to copy the 4 lines that do the test into each? I wouldn't do this for code I expected to stay around for long, but since this won't...

I don't really mind either way.

How long is a deprecation cycle for iris anyway? Will the code we are talking about will be present in iris 1.5, and then 1.6 too and removed in 1.7? Or removed in 1.6? Or later?

I think this is the closest we've ever coming to defining it: six months or two releases, whichever is the longer.

[rhattersley]

@rhattersley I decided to implement a decorator to handle the deprecation of the coords keyword in iris.plot.plot and iris.quickplot.plot. Hopefully this is easier to deal with once the deprecation period is over.

👍 Very nice. That's a pattern I can see us re-using - I'll link to it from that deprecation discussion in the hope that it eventually ends up in the mythical developer guide. 😉

[rhattersley]

@esc24 - anything you'd like to add before I press the green button with relish?

[ajdawson]

@esc24 - anything you'd like to add before I press the green button with relish?

@rhattersley make sure you leave time for me to quash this into a single commit!

[rhattersley]

@rhattersley make sure you leave time for me to quash this into a single commit!

Don't squash it on my account - the three commits up to 530b01f87ecbbd6cb9aff1c4f9c622cc852502e3 are fine by me.

[ajdawson]

I dislike commits that just say "review actions" or similar, but if you don't mind then I don't mind!

[ajdawson]

Needs a check on the length of the coords list to be consistent with the previous interface, which raises an error when more than 1 coord is in the list. I'll add this soon.

[ajdawson]

I've added a new commit to deal with some error handling issues. In deprecation mode: a check for the length of the coords keyword and a check that the coord matches the data. These were required for consistency with the previous interface. For the new interface, I added a check for compatibility of objects when two are provided. This is hopefully easier to understand than the matplotlib error one would get otherwise.

[esc24]

Comments aren't needed. Chances are they'll go out of date. Sorry to be pedantic, but could we maintain the alphabetical order?

[ajdawson]

I just put them next to one another with the comment there so it would be easier to clean up when the deprecation code is removed. I'll sort this out.

[rhattersley]

I've added a new commit to deal with some error handling issues.

It'd be lovely to have to some tests of those error handlers.

[esc24]

We tend to avoid multiple return statements as a coding style. Personally, I'd also have gone for the hasattr() rather that isinstance(). Not much in it though.

if hasattr(c, 'data'):
    data = c.data
elif hasattr(c, 'points'):
    data = _fixup_dates(c, c.points)
else:
    raise TypeError('...')

return data

[ajdawson]

I'm aware that numpy arrays have a data attribute too though so I thought it best to be specific here.

[esc24]

Seems very reasonable.

[ajdawson]

It'd be lovely to have to some tests of those error handlers.

Yes it would! Oversight on my part.

[esc24]

Why are you testing the boolean value of coord (I suspect this is pre-existing code)? I believe plot_defn.coords will be either be [coord] or [None] (I'm not certain though - sorry if I'm talking nonsense). If it is, this code can become:

u_object = None
if isinstance(v_object, iris.cube.Cube):
    plot_defn = _get_plot_defn(v_object, iris.coords.POINT_MODE, ndims=1)
    u_object, = plot_defn.coords
return u_object

or am I missing something?

[ajdawson]

I suspect this is pre-existing code

Yes this is based on the previous code, I'll look into it.

[esc24]

The conversion specifier and code aren't necessary here. I don't suppose they do any harm though.

[esc24]

Brilliant stuff. A few comments, but nothing major.

[ajdawson]

OK, I've added another commit with in response to @esc24 and @rhattersley's latest comments.

[esc24]

👍

[esc24]

@ajdawson - please squash ready for merging 😄

[ajdawson]

Squashed now.

[rhattersley]

🎉 Thanks again @ajdawson - a super job.

And thanks @esc24 for taking this over the line. 😀

[rhattersley]

Now ... who fancies doing all the other functions ... 😑