issue_comments

@dcherian @keewis any comments/suggestions on the currently unresolved comments? Or is this good to go?

Can we avoid the signature changes, seems like it creates more trouble than it solves.

I can put the positional ax args back for now, but I do think it would be better not to have in the signature, since it makes it look like you have to provide ax. The _plot2d deals with this by modifying the signature to hide it, but _dsplot doesn't. I do think it is unlikely that many users were using ax positionally, especially since this is never done in the examples, but it is certainly possible.

The positional u, v in dataset_plot.scatter were not used so I think that change is ok?

I agree many of these could be cleaned up. They do probably need a deprecation cycle though — i.e. checking for the previous args, warning if they're supplied, and then only later removing them.

The positional u, v in dataset_plot.scatter were not used so I think that change is ok?

Sounds good to me. I wish I'd thought of this when implementing quiver :)

Can we avoid the signature changes, seems like it creates more trouble than it solves.

I can put the positional ax args back for now, but I do think it would be better not to have in the signature, since it makes it look like you have to provide ax. The _plot2d deals with this by modifying the signature to hide it, but _dsplot doesn't. I do think it is unlikely that many users were using ax positionally, especially since this is never done in the examples, but it is certainly possible.

The positional u, v in dataset_plot.scatter were not used so I think that change is ok?

The RTD is failing with Sphinx parallel build error: nbsphinx.NotebookError: UndefinedError in examples/ERA5-GRIB-example.ipynb: 'nbformat.notebooknode.NotebookNode object' has no attribute 'tags' Doesn't seem like it would be related to this PR, but it was passing before...

Thanks @mathause for the review.

Looks good overall. There are two things I am unsure about:

1. You change the signature of ds.plot.scatter etc.. I think that's a worthwhile change but it needs a deprecation cycle - unless I am missing something? I'd suggest not doing that here but in a new PR. (I am also not entirely sure how to best do it...).

Yes, I suppose it would break it for anyone who was using ax positionally, but I am pretty sure no xarray docs examples do this, and I feel that the precedent from pandas, etc. is that ax is a kwarg.

1. I see the matplotlib: part of some of the references for the first time (e.g. py:meth:`matplotlib:matplotlib.figure.Figure.add_subplot\`) - this is used very inconsistently (already before). I am not sure what's correct.

It works both ways (I've been building locally to check things), but I can certainly add in the matplotlib:s if you would like. I suppose that makes it more clear that it is an external name, not part of xarray.