Improve docstring of contour #10968

timhoffm · 2018-04-06T01:20:13Z

PR Summary

As part of #10148: Docstring update for Axes.contour / Axes.contourf and related stuff.

anntzer · 2018-04-06T05:02:16Z

lib/matplotlib/contour.py

-        The first three arguments must be:
+        Call signature::
+
+            ContourSet(ax, levels, allsegs, [allkinds], **kwargs)


I would just pick some reasonable argument names (levels, allsegs and allkinds are fine, but you can also change them if you want as they were positional-only up to now) and transform this into a regular signature without custom parsing.

I agree with the proposed change, but will leave it to a separate PR.

It's probably simple enough. I did not touch this, because there's a bit of code paths to change with _process_args and maybe subclasses. I try to keep the docstring updates separated from really non-trivial code changes. This eases reviewing as well as backporting.

ImportanceOfBeingErnest · 2018-04-06T15:21:09Z

lib/matplotlib/contour.py

+            If an int *n*, use *n* data intervals; i.e. draw *n+1* contour
+            lines. The level heights are automatically chosen.
+
+            If array-like, draw contour lines the specified level values. 


If array-like, draw contour lines [at / on / through ?] the specified level values.

or

If array-like, contour the specified level values.

Since contour could be a verb?

ImportanceOfBeingErnest · 2018-04-06T15:23:51Z

lib/matplotlib/contour.py

+        locator : ticker.Locator subclass, optional
+            The locator is used to determine the contour levels if they
+            are not given explicitly via *levels*.
+            Defaults to `.MaxNLocator` is used.


Defaults to .MaxNLocator.

or

By default, .MaxNLocator is used.

ianthomas23 · 2018-04-06T15:27:19Z

@timhoffm If you are going to change contour/contourf documentation, you also need to change tricontour/tricontourf to be consistent.

ImportanceOfBeingErnest · 2018-04-06T15:41:52Z

lib/matplotlib/contour.py

+        Call signature::
+
+            contour([X, Y,] Z, [levels], **kwargs)
+
        :func:`~matplotlib.pyplot.contour` and
        :func:`~matplotlib.pyplot.contourf` draw contour lines and


Why does the Axes.contour documentation link to the pyplot.contour documentation (which is the same)?

Same in the Notes section (line 1792 ..)

Given the discussion in #10974 it might be sensible to keep the links to pyplot as the pyplot function docs have a gallery below them.

This same docstring is used both on the Axes and pyplot interfaces, because we don't want to maintain to almost identical versions. For this we have to pay the small price that both versions link to the same functions. We have the same issue with other passthrough functions. I've left the links as they are.

ImportanceOfBeingErnest · 2018-04-06T15:49:35Z

lib/matplotlib/contour.py

-        Optional keyword arguments:
+        Returns
+        -------
+        `matplotlib.contour.QuadContourSet`


This should actually link to the QuadContourSet docs,

:class:`~matplotlib.contour.QuadContourSet` object.

(modulo correct syntax)

ImportanceOfBeingErnest · 2018-04-06T15:58:56Z

I think it would be really great if the documentation of a function could link to some (or all?) examples from the gallery that use this function.

In this case those would be

gallery/images_contours_and_fields/contour_demo.html
gallery/images_contours_and_fields/contour_label_demo.html
gallery/images_contours_and_fields/contourf_demo.html
gallery/images_contours_and_fields/contour_corner_mask.html

I don't know at which point this would be specified, if it's in the docstring or some sphinx config file?

timhoffm · 2018-04-06T22:39:48Z

I would leave the gallery links to #10974.

jklymak · 2018-04-06T22:45:26Z

Yes, except I think where #10974 has left us is with manual gallery links for most methods...

ImportanceOfBeingErnest · 2018-04-06T22:55:24Z

I would leave the gallery links to #10974.

I would agree. In the moment writing this I was under the impression that this would be an easy change.

I think where #10974 has left us is with manual gallery links for most methods...

Still, there seems no obvious solution for manually providing those links, so this should probably done indepenently of the docstring updates proposed here.

In general it's great to see the documentation becoming more and more consistent. 👍

timhoffm · 2018-04-06T23:32:01Z

@ianthomas23 This is part of a larger effort #10148. tricontour/tricontourf will get their attention in due time.

ianthomas23 · 2018-04-09T06:38:49Z

@timhoffm Understood. As contour(f) and tricontour(f) share lots of documentation, I wouldn't want them to be out of sync for long.

jklymak · 2018-04-18T18:54:28Z

@timhoffm I'd be more comfortable approving this if you just did the change in tricontour at the same time. I know you have a list, but you could do out of order just this once. But the practical reason is that if there is lots of shared text, maybe it should be interpolated across the methods...

timhoffm · 2018-04-19T00:54:20Z

I don't care too much about same time. One improved docstring is better than none. 😄

I'm also a bit hesitant with too much interpolation as it's not too stable and probably wouldn't work for interpolating parameter sections until #10161 is solved.

dstansby · 2018-04-19T11:46:21Z

lib/matplotlib/contour.py

-
-        ::
-
-          contour(X,Y,Z)


Although this example list is long, I think it makes much more sense than the 1-liner that has replaced it above, so I would be keen to keep these lines.

You mean the stuff before Optional keyword arguments? Comparing old and new, this is mostly repetitive stuff:

Draw Z,

Draw Z with X, Y

Draw Z with parameter N

Draw Z with X, Y and parameter N

Draw Z with parameter V (which is just a more explicit control of the same property controlled by N)

Draw Z with X, Y and parameter N (which is just a more explicit control of the same property controlled by N)

It's much easier to say and understand:

Draw Z, optionally with X, Y and optionally with giving the levels.

I was really happy that I could boil this section down to quite a simple formula. To me it was really daunting before. I find it quite demotivating to read the doc at all, when I have to read a full screen before I have an idea, what parameters I could use.

One might add a sentence after the call signature, "where Z are the values, X, Y are the coordinates, ..". But then again, that's just a repetition of the parameter section, which is immediately following.

Can you explain the benefit you see in the long version?

I agree that there is a long list there, but I think contour([X, Y,] Z, [levels], **kwargs) is too terse, and confusing (can I do (X, Z)? (Z, levels)? e.g.). I think having a list of the allowable call signatures would be much better, and it could be boiled down to

contour(Z ...)

contour(X, Y, Z ...)

with some sort of explanation that levels can be provided before keyword arguments.

Well by definition, the square brackets mean optional. You can use or leave out every square bracket separately. So, X, Z does not work, but Z, levels does.

It's maybe a question if everybody knows this convention. I'd rather use a standard python function signature instead. However that's not possible with optional positional parameters, which are often used in matplotlib. Anyway, I'm still in favor of using the standard convention for optional parameters compared to verbosely giving multiple alternatives.

I could add a sentence "X, Y must be given both, or left out both." to the paramter description, if that adds additional clarity.

jklymak · 2018-04-28T19:42:49Z

I'm going to merge as an improvement. If we want to revert some of it because part have gotten too murky, thats fine, but I'm comfortable w/ the square bracket notation...

Improve docstring of contour Conflicts: lib/matplotlib/contour.py - conflict due to de-py2-ification - docstring conflicts, kept the version from master branch

anntzer reviewed Apr 6, 2018

View reviewed changes

dstansby added the Documentation label Apr 6, 2018

ImportanceOfBeingErnest reviewed Apr 6, 2018

View reviewed changes

timhoffm force-pushed the doc-contour branch from 5bed6fb to 668d1f2 Compare April 6, 2018 22:36

timhoffm force-pushed the doc-contour branch from 668d1f2 to b4f7b7a Compare April 6, 2018 23:04

timhoffm force-pushed the doc-contour branch from b4f7b7a to 7eca49f Compare April 8, 2018 00:30

timhoffm added the status: needs review label Apr 18, 2018

dstansby reviewed Apr 19, 2018

View reviewed changes

Improve contour docstring

33406f7

timhoffm force-pushed the doc-contour branch from 7eca49f to 33406f7 Compare April 26, 2018 21:15

jklymak approved these changes Apr 28, 2018

View reviewed changes

jklymak added this to the v3.0 milestone Apr 28, 2018

jklymak merged commit 0d4312d into matplotlib:master Apr 28, 2018

QuLogic removed the status: needs review label May 1, 2018

timhoffm deleted the doc-contour branch May 23, 2018 09:21

tacaswell modified the milestones: v3.0, v2.2.3 Aug 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve docstring of contour #10968

Improve docstring of contour #10968

timhoffm commented Apr 6, 2018

anntzer Apr 6, 2018

timhoffm Apr 6, 2018

ImportanceOfBeingErnest Apr 6, 2018

ImportanceOfBeingErnest Apr 6, 2018

ianthomas23 commented Apr 6, 2018

ImportanceOfBeingErnest Apr 6, 2018 •

edited

Loading

ImportanceOfBeingErnest Apr 6, 2018

timhoffm Apr 6, 2018

ImportanceOfBeingErnest Apr 6, 2018

ImportanceOfBeingErnest commented Apr 6, 2018

timhoffm commented Apr 6, 2018

jklymak commented Apr 6, 2018

ImportanceOfBeingErnest commented Apr 6, 2018

timhoffm commented Apr 6, 2018 •

edited

Loading

ianthomas23 commented Apr 9, 2018

jklymak commented Apr 18, 2018

timhoffm commented Apr 19, 2018

dstansby Apr 19, 2018

timhoffm Apr 19, 2018 •

edited

Loading

dstansby Apr 19, 2018

timhoffm Apr 19, 2018

jklymak commented Apr 28, 2018

Improve docstring of contour #10968

Improve docstring of contour #10968

Conversation

timhoffm commented Apr 6, 2018

PR Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianthomas23 commented Apr 6, 2018

ImportanceOfBeingErnest Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ImportanceOfBeingErnest commented Apr 6, 2018

timhoffm commented Apr 6, 2018

jklymak commented Apr 6, 2018

ImportanceOfBeingErnest commented Apr 6, 2018

timhoffm commented Apr 6, 2018 • edited Loading

ianthomas23 commented Apr 9, 2018

jklymak commented Apr 18, 2018

timhoffm commented Apr 19, 2018

Choose a reason for hiding this comment

timhoffm Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak commented Apr 28, 2018

ImportanceOfBeingErnest Apr 6, 2018 •

edited

Loading

timhoffm commented Apr 6, 2018 •

edited

Loading

timhoffm Apr 19, 2018 •

edited

Loading