Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve documentation on Axes position #9951

Merged
merged 1 commit into from Dec 12, 2017

Conversation

Projects
None yet
6 participants
@timhoffm
Copy link
Member

commented Dec 7, 2017

PR Summary

This PR improves the documentation of:

  • _AxesBase.get_position()
  • _AxesBase.set_position()
  • _AxesBase.reset_position()

Also, it does some minor cleanups on a few other docstrings.

Style questions

  • I would like to reference a documentation section on the figure coordinates. The existing code just references the Figure class, which is not too helpfull.
    Is there are basic description of the coordinate systems other/simpler than the Transforms tutorial?
  • Is there a code style for writing builtins like True and False in the docstring text? Such as in If *True*, return the original position. I've not found anything on this in the numpydocs or in https://github.com/matplotlib/matplotlib/blob/master/doc/devel/documenting_mpl.rst
@anntzer

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2017

I think we've sort of converged on using double backquotes for True, False and None, which is probably as good as anything (technically single backquotes would also work, as they'd link to, well, the docs entry on docs.python.org for these objects; it's just a matter of consistency).

@jklymak
Copy link
Contributor

left a comment

This looks good; suggested change below...

@timhoffm timhoffm force-pushed the timhoffm:doc-axes-position branch from 2d797a4 to b449346 Dec 9, 2017

@timhoffm

This comment has been minimized.

Copy link
Member Author

commented Dec 9, 2017

I've now set True in backqoutes.

@jklymak Thank you for the review. You write "suggested change below". Sorry, I don't know where "below" is. I haven't found the suggested change. Could you please hint, where I have to look?

@WeatherGod

This comment has been minimized.

Copy link
Member

commented Dec 9, 2017

@jklymak

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2017

oh hmm guess I hit the wrong button somewhere. I’ll look again tonight.

@jklymak
Copy link
Contributor

left a comment

Second time's the charm?

The expected shape of ``pos`` is::
There are two position variables:

This comment has been minimized.

Copy link
@jklymak

jklymak Dec 9, 2017

Contributor

This was the part I found a bit confusing, and I understand what you are talking about. Maybe:

Axes have two position attributes: the 'original' position is the position allocated for the axes; the 'active' position is the position the Axes is actually drawn at. These positions will usually be the same except if the Axes has its aspect ratio set by .apply_aspect.

Reset the original position to the active position. This resets the
a possible position change due to aspect constraints.
For an explanation of the positions see `.set_position`.
"""

This comment has been minimized.

Copy link
@jklymak

jklymak Dec 9, 2017

Contributor

This is actually backwards (as was the original docstring!). Should be

Reset the 'active' position to the 'original' position, undoing the effect of ~.set_aspect.

This comment has been minimized.

Copy link
@timhoffm

timhoffm Dec 10, 2017

Author Member

Changed to: "Reset the active position to the original position."

It's not really undoing the effect of set_aspect, but of apply_aspect, so I don't want to adapt the second half-sentence. But explaining this is a bit too much for the docstring.

As written in #9964:
Overall, I think we would need a dedicated help page outside the docstrings to explain, coordinates, scaling and the related terminology, which is not always consistent and/or self-explanatory.

This comment has been minimized.

Copy link
@jklymak

jklymak Dec 10, 2017

Contributor

I think users will think of it as set_aspect. I also think that set_aspect is the sensible place to define all of this, so a link back to it would be helpful to users. apply_aspect could also be mentioned.

@timhoffm timhoffm force-pushed the timhoffm:doc-axes-position branch from b449346 to 5c9b102 Dec 10, 2017

@jklymak jklymak merged commit 40eb83f into matplotlib:master Dec 12, 2017

8 checks passed

ci/circleci: docs-python27 Your tests passed on CircleCI!
Details
ci/circleci: docs-python35 Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing 14d68df...5c9b102
Details
codecov/project/library 61.7% (target 50%)
Details
codecov/project/tests 98.86% remains the same compared to 14d68df
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details

@QuLogic QuLogic added this to the v2.2 milestone Dec 12, 2017

@timhoffm timhoffm deleted the timhoffm:doc-axes-position branch Dec 12, 2017

@QuLogic QuLogic modified the milestones: needs sorting, v2.2.0 Feb 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.