issue_comments

It would be nice to factor out some of the helper functions into plot.utils or elsewhere, and especially to use the @_plot2d decorator, but there is not need to squeeze that into this PR.

Thanks, @TomNicholas. My original plan and attempts was to use _plot2d. But because the return type originally was so different to the other plot functions (list of pathcollections vs pathcollections) it was impossible to use it without pretty much if-casing the entire _plot2d. I also feel _plot2d makes it pretty hard to read and understand the code.

I think it might be doable now that the function only returns pathcollection. I'll take a stab at that again in a different PR.

If you're worried about this not being fully robust until other plotting methods are condensed into one code path then I would just suggest not making it callable as a plot method yet (or renaming to _scatter as you suggested). But I also think it's fine to release as is.

Ok, I'll rename it to a private name. But I think it's good that it's still callable, so that others can easily try it out without any promises that the API will stay the same. I was also considering changing/removing some variable names related to legend compared to the dataset version, so it's good to let that it be private until that's been figured out.

If it seems safer we could make this non-public, da.plot._scatter? This is not being tested as properly as ds.plot.scatter at the moment so I might've missed some edge cases, but once we use the same code path for both of them this should be more reliable.

I took a quick look and left some minor comments. I do wonder whether it's easier to call DataArray.to_dataset().plot.scatter and port the wonderful legend code to dataset_plot.py

My idea was to slowly move all the code to plot.py and then use the same wrapper function as in #4820, this is what I'm already doing in my local version at least.

I think this is ready for some different eyes. Feel free to try it out @dcherian if you have the time.

Gotten the facetgrids to work again, fortunately it didn't require that much changes. The 3d versions chooses the axes limits a little weirdly though.

ds.plot.scatter(x="A", y="B", col="x", hue="w", markersize="y", add_legend=False):

Seems CI Additional / ubuntu-latest py37-min-all-deps (pull_request) doesn't install the 3d parts of matplotlib that's a bit unfortunate I think.

Edit: Seems this functionality got added in 3.2: mpl_toolkits.mplot3d is always registered by default now. It is no longer necessary to import mplot3d to create 3d axes with https://matplotlib.org/3.2.0/api/api_changes.html

figlegenddoesn't work that great within subplots though right? When I tried subplots the legends were just added on top of each other. I think ax.legend is a better, but it seems rather difficult to fit it outside. I've tried a few variants with bbox_to_anchor but it quickly gets non robust when deviating from the defaults.

I've been trying to merge the legends together though . So then the legend will attempt to find the best places automatically so that helps a little bit: