Since #1109 was merged the tests need minor modifications. It doesn't look like this will cater for 1d plots such as iplt.scatter over a map. We have an example in the gallery where a colorbar is used for a scatter plot but it isn't on a map so wouldn't be affected, but I can envisage it for trajectories etc. so it would be worth looking into.

Since #1109 was merged the tests need minor modifications. It doesn't look like this will cater for 1d plots such as iplt.scatter over a map. We have an example in the gallery where a colorbar is used for a scatter plot but it isn't on a map so wouldn't be affected, but I can envisage it for trajectories etc. so it would be worth looking into.

Seems to work for me:
http://nbviewer.ipython.org/gist/cpelley/b0186e28b2357b34de1d

Over to you @esc24

Seems to work for me:

Because @esc24 has added a commit to make it work since I made that comment.

Well spotted @cpelley and @ajdawson. I pushed it up late yesterday along with a rebase to get the skip plot stuff so the timeline is a little misleading on github.

I just noticed that the first argument of _geoaxes_draw_method_and_kwargs() is now never used. Perhaps it would be better to modify the function (and its name) to do only the parts we now need?

Now that the draw method is not explicitly used, it is extremely unclear from the code how we end up with a draw method that belongs to GeoAxes at all. plt.contour etc. will extract the draw method from the current axes, so the current axes must be being set to a GeoAxes instance somewhere, but how and where this occurs is very opaque. I introduced _geoaxes_draw_method_and_kwargs() in 33a763a as it suited the need there, but with hindsight I should have done it differently.

Could you take a stab at restructuring this area of the code? I think we just need to make it clear (and explicit) how we get a GeoAxes from plt. call. Let me know if you need a better explanation, I found that quite difficult to explain properly.

Could you take a stab at restructuring this area of the code?

I was trying to keep this change a light touch bug fix, rather than a structural change given the complex (some might say convoluted) way the plot code works. I agree it needs an overhaul, but I'd rather not do it as part of a bug fix. Having said that I'm happy to modify and rename the helper functions so it's clearer how one ends up with a geoaxes instance.

Having said that I'm happy to modify and rename the helper functions so it's clearer how one ends up with a geoaxes instance.

That'd be a good start, please go ahead. My concern is that leaving the code in in its current state will lead to other modifications to the plot code being poorly designed, as it is quite confusing. It's be nice to get this straightened out earlier rather than later. Whatever you can do to help would be great!

@ajdawson - I've a numbers of small changes that hopefully make the logic clearer.

@esc24 - that looks a lot better, I'm happy with these changes.

@esc24 - could you squash the 3 commits on this branch to keep the history on master as clean as possible?

Thanks @esc24.