issue_comments

Tests seem to be failing due to lint errors, see https://github.com/pydata/xarray/pull/2277#issuecomment-447607250

I haven't reviewed this in detail, but I wonder if we shouldn't just merge it in. This has tests and docs, and most of this has been looked over by both @dcherian and @yohai. Also, it's in a pretty self-contained part of the code (plotting). So it seems pretty slow risk to me.

Could we call this mark_size instead of scatter_size? The later sounds a little awkward to me.

I'm not sure about this. Legends and colorbars are different and the DataArray.plot API has both switches: add_legend and add_colorbar.

Is there a generic term that covers both discrete legends and colorbars?

ggplot2 uses "guide" but I haven't seen that elsewhere. Still, add_guide=False could be a good way to write this genericly.

This generally looks good to me...

I guess we still use s= for indicating marker size?

The reason I started this PR in the first place is that I happened to do relatively simple scatter plots quite often, so I thought it'd be handy. but for more elaborate ones I would use a dedicated tool like seaborn.

:+1: This seems like a reasonable place to draw the line to me.

This is looking really nice. Coincidentally, the new version of Seaborn was released today, and has a whole new doc section on "relational plots": http://seaborn.pydata.org/tutorial/relational.html#relational-tutorial

It's probably worth a look over to see if it has good ideas worth stealing, or if we want to make intentional deviations from its behavior in xarray.

It is possibly worth taking a look at the recent (not yet released) scatterplot (https://github.com/mwaskom/seaborn/pull/1436) and relplot (https://github.com/mwaskom/seaborn/pull/1477) additions to Seaborn.

seaborn.scatterplot will use hue/size rather than c/s, which is definitely more readable. One hazard is that it it means that the size argument from seaborn.FacetGrid needs to be renamed to avoid name conflicts -- it's now becoming height. Unfortunately we would also need to rename the size argument if we followed Seaborn's example.

I guess I can see the virtue in sticking with matplotlib's old c/s names, but those really are terrible names. Maybe hue/mark_size would be a good compromise? Or we could systematically switch size -> height elsewhere like Seaborn.

You should try to emulate the pandas scatter api as much as possible (which itself emulates the matplotlib api). That means using the c keyword instead of hue and also implementing s for size.

I disagree here. In many cases, we already follow the naming conventions from Seaborn instead, which uses meaningful names. c and s are pretty meaningless.