Clean up and enhance plot method docstrings #5285

zmoon · 2021-05-10T01:49:30Z

Passes pre-commit run --all-files
User visible changes (including notable bug fixes) are documented in whats-new.rst

Summary:

use code-literal style for variables (arguments) and other Python identifiers
fix some type specifications
add more references where helpful
italic for x and y when talking about axes, like in math
correct a bit of grammar stuff
tell which Matplotlib function is being wrapped at the top
capitalize Matplotlib when referring to the library in general, since that is how they do it in their docs, like xarray is lowercase unless starting the sentence
~~remove ax from the signature of the _dsplot functions (unlike the _plot2d ones, it shows in the built docs)~~ (save for later PR, so can do deprec. cycle because users might have been using ax positionally)
fix matplotlib colormap name Napoleon type alias
remove unused u and v from signature of Dataset.scatter

decided to adopt the `*x*` style used in it in the previous one (plot.line)

confusing to have it showing in there

max-sixty

Thanks @zmoon ! These sorts of clean-ups of docs are helpful.

@keewis knows more about when we use rst-style in docstrings, so I'll let him comment.

For another PR — if you wanted to add some of these types as python types, that would also be welcome.

max-sixty · 2021-05-10T02:37:59Z

xarray/plot/dataset_plot.py

+        Variable by which to color scattered points or arrows.
    hue_style: str, optional
-        Can be either 'discrete' (legend) or 'continuous' (color bar).
+        Can be either ``'discrete'`` (legend) or ``'continuous'`` (colorbar).


@keewis would you mind opining on the best format for literals in docstrings?

you mean, str literals? We're currently not consistent in our code base so I think we're free to choose. If we choose to use double-backticked quoted strings (which looks better in the rendered version and slightly decreases the readability in pydoc / source code) I would vote for double quotes (") instead of single quotes ' because that's what we use in the code.

Also, would it make sense to replace str with {"discrete", "continuous"}?

I think a case for single-quoted strings (+ double-backticked) in docs is that Python uses single quotes for the repr of strings. But in this PR I just picked one and tried to make it consistent. I am happy to change to double.

I tweaked the discrete vs continuous thing a bit in 78c77ed if anyone wants to check what I wrote for hue_style.

@keewis

Per @keewis's review of #5293

mathause

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

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...).
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.

xarray/plot/plot.py

xarray/plot/dataset_plot.py

xarray/plot/plot.py

zmoon · 2021-05-13T12:51:16Z

Thanks @mathause for the review.

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

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.

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.

Co-authored-by: Mathias Hauser <mathause@users.noreply.github.com>

zmoon · 2021-05-13T15:37:57Z

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...

xarray/plot/plot.py

dcherian

Thanks @zmoon. You should add a whats-new note to take credit for this. It's a great improvement.

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

xarray/plot/plot.py

zmoon · 2021-05-13T17:17:35Z

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 · 2021-05-13T17:20:22Z

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 :)

max-sixty · 2021-05-13T18:03:55Z

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.

keewis

thanks, @zmoon, and sorry for the late reply. I think we should change the signature in a new PR (using override_signature, similar to what _plot2d does), and I'm not entirely sure whether ' or " is better, but otherwise this looks good to me.

keewis · 2021-05-10T23:13:27Z

xarray/plot/dataset_plot.py

+        Variable by which to color scattered points or arrows.
    hue_style: str, optional
-        Can be either 'discrete' (legend) or 'continuous' (color bar).
+        Can be either ``'discrete'`` (legend) or ``'continuous'`` (colorbar).


you mean, str literals? We're currently not consistent in our code base so I think we're free to choose. If we choose to use double-backticked quoted strings (which looks better in the rendered version and slightly decreases the readability in pydoc / source code) I would vote for double quotes (") instead of single quotes ' because that's what we use in the code.

Also, would it make sense to replace str with {"discrete", "continuous"}?

keewis · 2021-05-10T23:20:35Z

xarray/plot/dataset_plot.py

+        Note: if ``cmap`` is a seaborn color palette,
        ``levels`` must also be specified.
-    colors : color-like or list of color-like, optional
+    colors : str or array-like of color-like, optional


I don't think that's right, color-like doesn't have to be a str, it can also be a tuple of floats describing RGB or RGBA values:

Suggested change

colors : str or array-like of color-like, optional

colors : color-like or array-like of color-like, optional

From pyplot.contour:

As a shortcut, single color strings may be used in place of one-element lists, i.e. 'red' instead of ['red'] to color all levels with the same color. This shortcut does only work for color strings, not for other ways of specifying colors.

I think I tried and found it to be the case with xarray.plot.contour as well. That is, it can be a string, or it can be an array-like of color-like values.

Edit: just checked it again. For example, colors=(0, 0, 0) raises ValueError: Invalid RGBA argument: 0, but colors=[(0, 0, 0)] or colors="black" are fine.

ds.plot.scatter(..., colors=(1, 0, 0)) does not complain, but I can't get it to change the color of the scatter marks (not even with colors="r") so I might be missing something.

Yeah, it seems like the colors argument is not used in ds.plot.scatter. But no error is raised, colors is just silently dropped (ax.scatter would raise AttributeError if it weren't) in most cases. I did find that you can get the marker colors to change by passing color or c with a discrete hue_style.

import xarray as xr import numpy as np ds = xr.tutorial.scatter_example_dataset() ds["c"] = (ds.A.dims, np.random.choice(("r", "g", "b"), ds.A.shape)) ds.plot.scatter("A", "B", hue="c", c=(0, 0, 0)) # works but prints (doesn't raise) warning ds.plot.scatter("A", "B", hue="c", color=(0, 0, 0)) # works

However, if you have a continuous hue_style, passing c is ignored (since this is done internally), and passing color raises ValueError since it conflicts with c.

It seems like at the moment, colors is really only intended to be used with contour(f) for levels, like how it is in Matplotlib. Maybe in the future it could be used to allow passing colors to be used in the color cycle for discrete hue_style, but it doesn't currently do that. So for now, maybe in the docstring we should note this current behavior (doing nothing or raising error).

~~levels is also unused in _dsplot functions, maybe could be removed~~ actually I was able to get colors to sort of work with discrete hue_style by also providing levels, but levels only makes sense for numeric type:

ds["c2"] = (ds.A.dims, np.random.choice((1, 2, 3), ds.A.shape)) ds.plot.scatter("A", "B", hue="c2", colors=["r", "g", "b"], levels=[1, 2, 3, 4]) # works (rgb) ds.plot.scatter("A", "B", hue="c2", hue_style="discrete", colors=["r", "g", "b"]) # `colors` does nothing

I think we can address this by raising appropriate errors (or implementing it) in a future PR.

Ok, I am interested in working on this colors stuff. If that is ok, I will open new issue from my comment above and go from there.

sounds great.

keewis · 2021-05-13T15:44:34Z

xarray/plot/dataset_plot.py

+        If provided, create a new figure for the plot with the given size:
+        *height* (in inches) of each plot. See also: ``aspect``.
+    norm : matplotlib.colors.Normalize, optional
+        If the ``norm`` has ``vmin`` or ``vmax`` specified, the corresponding kwarg


Suggested change

If the ``norm`` has ``vmin`` or ``vmax`` specified, the corresponding kwarg

If ``norm`` has ``vmin`` or ``vmax`` specified, the corresponding kwarg

How about: the :py:class:`~matplotlib.colors.Normalize` instance?

wouldn't it be easier to just refer to the object by name? I don't think we need the link because it's already in the type spec.

Yeah, probably better to keep it simple.

I guess "must be None" isn't really true, because it can also be the same value as in the norm. But maybe that isn't important. But maybe there is a better way to word this description.

dcherian · 2021-05-14T14:44:39Z

xarray/plot/dataset_plot.py

-              This is the default for non-numeric `hue` variables.
-            - for "continuous",  build a colorbar
+        smaller arrows.
+    add_guide: bool, optional, default: True


This should also control the quiverkey for quiver.

That seems like it could make sense.

zmoon · 2021-05-15T14:01:47Z

xarray/plot/dataset_plot.py

-        Can be either 'discrete' (legend) or 'continuous' (color bar).
+        Variable by which to color scatter points or arrows.
+    hue_style: {'continuous', 'discrete'}, optional
+        How to use the ``hue`` variable:


Maybe "how to color" instead of "how to use"?

zmoon · 2021-05-18T13:34:43Z

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

dcherian

This is a great improvement. Thanks @zmoon!

I think we can address any remaining minor comments in a future PR.

zmoon added 10 commits May 9, 2021 16:38

plot.plot, plot.line

eae252d

plot.step

d83a6b0

decided to adopt the `*x*` style used in it in the previous one (plot.line)

plot.hist; emphasize width/height so far

2777557

plot._plot2d

83a645c

the ones wrapped with _plot2d

f7bd01e

plot.dataset_plot

c2ab27d

Remove ax from sigs of _dsplot-wrapped fns

7c188c6

confusing to have it showing in there

consistent type listing for ax

88897eb

pre-commit

16e1ac9

Add colormap reference link for _dsplot too

fb96fe2

max-sixty reviewed May 10, 2021

View reviewed changes

zmoon mentioned this pull request May 12, 2021

FacetGrid docstrings #5293

Merged

2 tasks

max-sixty requested a review from keewis May 12, 2021 23:05

zmoon added 2 commits May 12, 2021 22:59

Fix "matplotlib colormap name" Napoleon type alias

6c543de

Use :py:func: instead of :func:

f0c6bbb

Per @keewis's review of #5293

mathause reviewed May 13, 2021

View reviewed changes

Apply some suggestions from code review verbatim

8216d3c

Co-authored-by: Mathias Hauser <mathause@users.noreply.github.com>

zmoon changed the title ~~Clean up and consistent-ify plot method docstrings~~ Clean up and enhance plot method docstrings May 13, 2021

Incorporate other @mathause suggestions; other fix

4328a72

zmoon commented May 13, 2021

View reviewed changes

xarray/plot/plot.py Outdated Show resolved Hide resolved

dcherian reviewed May 13, 2021

View reviewed changes

xarray/plot/plot.py Outdated Show resolved Hide resolved

xarray/plot/plot.py Outdated Show resolved Hide resolved

keewis reviewed May 13, 2021

View reviewed changes

zmoon added 2 commits May 13, 2021 21:13

Address recent suggestions

78c77ed

Add a whats-new

9827e33

dcherian reviewed May 14, 2021

View reviewed changes

zmoon commented May 15, 2021

View reviewed changes

Simplify reference to the color norm

130287d

dcherian approved these changes May 18, 2021

View reviewed changes

dcherian merged commit 49aa235 into pydata:master May 18, 2021

zmoon mentioned this pull request May 18, 2021

colors in Dataset plot methods #5337

Open

	colors : str or array-like of color-like, optional
	colors : color-like or array-like of color-like, optional

	If the ``norm`` has ``vmin`` or ``vmax`` specified, the corresponding kwarg
	If ``norm`` has ``vmin`` or ``vmax`` specified, the corresponding kwarg

Uh oh!

Clean up and enhance plot method docstrings #5285

Clean up and enhance plot method docstrings #5285

Uh oh!

Conversation

zmoon commented May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

max-sixty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zmoon May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mathause left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zmoon commented May 13, 2021

Uh oh!

zmoon commented May 13, 2021

Uh oh!

Uh oh!

dcherian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zmoon commented May 13, 2021

Uh oh!

dcherian commented May 13, 2021

Uh oh!

max-sixty commented May 13, 2021

Uh oh!

keewis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zmoon May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zmoon May 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zmoon May 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

zmoon commented May 10, 2021 •

edited

Loading

zmoon May 13, 2021 •

edited

Loading

zmoon May 13, 2021 •

edited

Loading

zmoon May 15, 2021 •

edited

Loading

zmoon May 18, 2021 •

edited

Loading

zmoon May 15, 2021 •

edited

Loading