API: change add_basemap to not return the received ax object

[jorisvandenbossche]

This is a proposal to change the return value of add_basemap from ax to the Image artist created by the function. Some reasoning (as it is a breaking change):

• returning ax is useless (you already pass the ax objects, so you already have this object named) and also potentially confusing (is this a different ax than the one passed in?)
• sometimes it can be useful to have the matplotlib artist object that was created (eg for eg for adding a colorbar for it, although that is not relevant for tile backgrounds, but there might be other things as well).
  You can get this eg by inspecting ax.get_children(), but that is always a bit tricky. Having it returned from the function is easier (and in 99% of the cases the user can simply ignore this return value anyway).
• at some point this is what Thomas Caswell (matplotlib lead developer) recommended for functions extending matplotlib / addings things to a plot: accepting an ax, returning the created artists. And it is also what many matplotlib's methods do (eg ax.imshow).

(one question might be if we also want to return the Text artist from the attribution)

[coveralls]

Coverage remained the same at 92.938% when pulling 8954993 on jorisvandenbossche:add_basemap-return into fcf341e on darribas:master.

[ljwolf]

Is there a reference on @tacaswell's suggestion?

I thought returning a new axis (if the input ax=None) or the modified axis (if the input ax is not None) is a pretty common pattern:

• geopandas (which it's really closely paired with)
• seaborn, which is probably the most used additional plotting library. I also see that seaborn "will not change" from this pattern back in 2015. Maybe that's different now, but right I think it returns axes for axes-level functions.
• yellowbrick, which seems to be built pretty strongly around returning axes.

I definitely agree with the second point and see how the first may be ambiguous. However, choosing to return the result of the imshow rather than an axis seems like we'd be breaking a convention that's common in other packages? A reference on that recommendation would be rad!

[jorisvandenbossche]

Is there a reference on @tacaswell's suggestion?

I have found this: pandas-dev/pandas#4020 (comment) (although I thought there was also a more recent discussion about it)

The main difference with all the other ones you mention (pandas, geopandas, seaborn) is that they all can create an ax. While here, add_basemap (at least at the moment), never creates an ax, and only adds something to an already existing ax.

[jorisvandenbossche]

Mainly what I would like to avoid is that people write:

...
ax = ctx.add_basemap(ax)

as re-assigning to ax is simply useless (but which we already started to do ourselves in the tests).

We could also return nothing for now, which is the most conservative and keeps the option open for later to return an artist (if there is a good use case) or ax (if we would start creating one).

[darribas]

As I see it, I can't think of a case where add_basemap would create a new ax. For that, the library has other methods (e.g. bounds2img or the Place API) so I'm starting to lean over the artist. Given we're now hedaed for a new rc release, this might be a good place to try it?

[jorisvandenbossche]

Since we would still have discussion about returning only image artist or both image and text artists (not sure what the recommended way is then, if your function adds multiple items to the ax ..), we could also leave it to "not returning the ax" (returning nothing) to avoid breaking code later if we decide to return an artist.

[jorisvandenbossche]

Updated the PR to return nothing for now, so we can further discuss what to return exactly, and if needed start to actually return something in the future.

[tacaswell]

Is there a reference on @tacaswell's suggestion?

I have made this suggestion is many places, including in the docs (https://matplotlib.org/tutorials/introductory/usage.html#coding-styles)