Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Improve docstring of contour#10968

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
jklymak merged 1 commit intomatplotlib:masterfromtimhoffm:doc-contour
Apr 28, 2018
Merged

Conversation

timhoffm
Copy link
Member

PR Summary

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

The first three arguments must be:
Call signature::

ContourSet(ax, levels, allsegs, [allkinds], **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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?

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Defaults to.MaxNLocator.

or

By default,.MaxNLocator is used.

@ianthomas23
Copy link
Member

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

Call signature::

contour([X, Y,] Z, [levels], **kwargs)

:func:`~matplotlib.pyplot.contour` and
:func:`~matplotlib.pyplot.contourf` draw contour lines and
Copy link
Member

@ImportanceOfBeingErnestImportanceOfBeingErnestApr 6, 2018
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Why does theAxes.contour documentation link to thepyplot.contour documentation (which is the same)?

Same in the Notes section (line 1792 ..)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This should actually link to the QuadContourSet docs,

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

(modulo correct syntax)

@ImportanceOfBeingErnest
Copy link
Member

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
Copy link
MemberAuthor

I would leave the gallery links to#10974.

@jklymak
Copy link
Member

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

@ImportanceOfBeingErnest
Copy link
Member

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
Copy link
MemberAuthor

timhoffm commentedApr 6, 2018
edited
Loading

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

@ianthomas23
Copy link
Member

@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
Copy link
Member

@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
Copy link
MemberAuthor

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.


::

contour(X,Y,Z)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link
MemberAuthor

@timhoffmtimhoffmApr 19, 2018
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

You mean the stuff beforeOptional keyword arguments? Comparingold andnew, 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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I agree that there is a long list there, but I thinkcontour([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 thatlevels can be provided before keyword arguments.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Well by definition, the square brackets meanoptional. You can use or leave out every square bracket separately. So,X, Z does not work, butZ, 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.

@jklymakjklymak added this to thev3.0 milestoneApr 28, 2018
@jklymak
Copy link
Member

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

@jklymakjklymak merged commit0d4312d intomatplotlib:masterApr 28, 2018
@timhoffmtimhoffm deleted the doc-contour branchMay 23, 2018 09:21
@tacaswelltacaswell modified the milestones:v3.0,v2.2.3Aug 5, 2018
tacaswell pushed a commit to tacaswell/matplotlib that referenced this pull requestAug 5, 2018
Improve docstring of contourConflicts:lib/matplotlib/contour.py          - conflict due to de-py2-ification          - docstring conflicts, kept the version from master branch
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@anntzeranntzeranntzer left review comments

@dstansbydstansbydstansby left review comments

@ImportanceOfBeingErnestImportanceOfBeingErnestImportanceOfBeingErnest left review comments

@jklymakjklymakjklymak approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v2.2.3
Development

Successfully merging this pull request may close these issues.

8 participants
@timhoffm@ianthomas23@ImportanceOfBeingErnest@jklymak@anntzer@dstansby@tacaswell@QuLogic

[8]ページ先頭

©2009-2025 Movatter.jp