issue_comments

Ah, I thought it might be something like that when you mentioned invariance earlier. Thanks!

@headtr1ck would you mind explaining a bit more what the complaint was? I had string keys and str is hashable, so still a bit confused.

Yes mypy is pleased with encoding: dict[Hashable, dict[str, Any]] (or if pass encoding as dict literal instead of creating a variable beforehand).

Some thoughts: * I am not too sure about the short description for join='override' that I added for Dataset.merge(). * Looks like there is a compat='minimal' option as well that is not documented. * If this is desired, I would also change to single quoted strings wrapped in `` for the option descriptions

Also I see the xr.merge() one has a different order between the set of options and the descriptions, could fix that too.

Some references: * not listed in array API attributes * documented in numpy * slightly documented in dask * slightly more documented in cupy

Also, I guess it is pretty self-explanatory, but perhaps some documentation could be added, maybe at least a reference to numpy.ndarray.nbytes.

Thanks @max-sixty. ~~I think~~ it was with %matplotlib widget (ipympl) that I got the behavior I described. With %matplotlib inline (default inline backend) the figures get closed[^1] at the end of a cell, so the two plot commands would have to be in the same cell to get an overlay, which is fine. But with ipympl, this is not the case so you have to be more careful.

[^1]: You can disable this behavior with %config InlineBackend.close_figures=False and get an overlay from plot commands in separate cells, but then you have to manually refresh the plot it seems.

Sorry, not sure. I don't recall noticing it before.

Yeah, having a kwarg that is noted to be specifically for the hue variable could make it more clear. It doesn't seem like the Dataset plot methods need to have both cmap and colors. They could be replaced by one, like palette, or keep just colors? But I guess with colors and levels you can make colormaps on the fly, which is convenient. And this does work for plot.scatter, but the hue variable has to be numeric.

@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?

@dcherian in some sense this is a "user visible change", but quite minor. I wasn't sure if I should have added to What's New or not.

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.

I agree with @zmoon that other API pages might benefit from these links, too, as long as we remember to sufficiently describe the link so the entry is still useful in text mode.

@keewis just wondering, did you see my comments?. It doesn't matter in this case, but it might be relevant in others.

Currently renders like this, which I don't think is intended.

numpydoc See Also section is really intended for references to code; it is taking http as the identifier (and linking to Python std lib's http package).

Edit: looks like there is an .. _io.zarr: anchor, so :ref:`io.zarr` should work? (or :doc:`io` for just the page)