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

Doc changes in add_subplot and add_axes #11459

Merged
merged 1 commit into from Jun 30, 2018

Conversation

fredrik-1
Copy link
Contributor

I did some changes in the documentation for Figure.add_subplot, Figure.add_axes and their pyplot equivalents. Added some parameters (in the documentation) and tried to do them consistent to each other. I also did some small changes in subplots, added a num parameter and changed the example section to :: instead of >>>

I also changed the docstring in axes._base.__init__ and made it visible in the documentation.

The links to the SubplotBase and some links in the axes kwargs list don't work but that would be fixed with my other PRs.

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@ImportanceOfBeingErnest
Copy link
Member

You ticked "Code is PEP 8 compliant" but it isn't. There are some line which are too long.

I guess it would simplify review if you linked to the produced documentation. Where exactly is the SubplotBase documentation?

@fredrik-1
Copy link
Contributor Author

I don't know how to link to the produced documentation.

The SubplotBase documentation are in the docstrings and they can be seen in the html documentation if the whole axes module is included in the documentation. with for example #11454.

@jklymak
Copy link
Member

jklymak commented Jun 19, 2018

Drill down in the circleCI tests - it gives you a link to the documentation. Yes, we can do it, but if 6 reviewers have to click bunch of links, its nice to provide a shortcut. Also in this case, its not 100% clear what you think changed, and a link will be worth 1000 words.

@fredrik-1
Copy link
Contributor Author

Thanks, I will try that but I cant do that before the tests are done.

@fredrik-1
Copy link
Contributor Author

So here are links to the changed documenation:
add_subplot
add_axes
subplots
plt.subplot
plt.axes
plt.subplots
Axes

@jklymak
Copy link
Member

jklymak commented Jun 19, 2018

You still have a pep8 error! Lines need to be 79 characters long (no idea why 79 and not 80)

Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

Lots of suggested changes here, but this seems very helpful overall. Thanks a lot!

corner and increases to the right.

A 3-digit integer, *pos*, can be used if all integers are less
than 10.
Copy link
Member

Choose a reason for hiding this comment

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

pos is a three digit integer, where the first digit is the number of rows, the second the number of columns, and the third the index of the subplot. i.e. fig.add_subplot(235) is the same as fig.add_subplot(2, 3, 5). Note that all integers must be less than 10 for this form to work.

'polar', rectilinear'}, optional
The projection type of the axes.
'polar', 'rectilinear', str}, optional
The projection type of the `~.axes.Axes`. *str* is the name of
Copy link
Member

Choose a reason for hiding this comment

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

The projection type of the subplot (~.axes.Axes).

(or introduce the fact that this method produces an Axes earlier. Otherwise the naive user is going to wonder what you are talking about...

def add_subplot(self, *args, **kwargs):
"""
Add a subplot.
Add a subplot to the figure.
Copy link
Member

Choose a reason for hiding this comment

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

Add an `~.axes.Axes` to the figure as part of a subplot arrangement.  


polar : boolean, optional
If True, equivalent to projection='polar'.

sharex, sharey : `~.axes.Axes`, optional
Make the returned axes share the x or y `~matplotlib.axis`
with the input axes.
Copy link
Member

Choose a reason for hiding this comment

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

Share the x or y ~matplotlib.axis with sharex and/or sharey. The axis will have the same limits, ticks, and scale as the axis of the shared axes.


Returns
-------
axes : Axes
The axes of the subplot.
axes : an `.axes.SubplotBase` subclass of `~.axes.Axes` (or a subclass \
Copy link
Member

Choose a reason for hiding this comment

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

.axes.SubplotBase didn't link. I think axes._subplots.SubplotBase?

new axes), you must use a unique set of args and kwargs. The axes
*label* attribute has been exposed for this purpose: if you want
two axes that are otherwise identical to be added to the figure,
make sure you give them unique labels.
Copy link
Member

Choose a reason for hiding this comment

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

What? Thats true for this method as well?

Choose a reason for hiding this comment

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

Yes it is.

rows and *ncols* columns. *index* starts at 1 in the upper left
corner and increases to the right.

A 3-digit integer, *pos*, can be used if all integers are less
Copy link
Member

Choose a reason for hiding this comment

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

As above...

If *nrows*, *ncols* and *index* are all less than 10, they can also be
given as a single, concatenated, three-digit number.
sharex, sharey : `~.axes.Axes`, optional
Make the returned axes share the x or y `~matplotlib.axis`
Copy link
Member

Choose a reason for hiding this comment

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

As above

rectilinear base class `~.axes.Axes` can be found in
the following table but there might also be other keyword
arguments if another projection is used.
%(Axes)s
Copy link
Member

Choose a reason for hiding this comment

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

This didnt render... (I think you need the interp decorator)

plt.subplot(211)

If you do not want this behavior, use the `.Figure.add_subplot` method
or the `.pyplot.axes` function instead.
Copy link
Member

Choose a reason for hiding this comment

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

?? You just said these also have that behaviour....

Choose a reason for hiding this comment

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

The confusing bit is that plt.subplot will kill any other subplot that's below it, unless that subplot has been created with identical *args,**kwargs.

Say you have a subplot

ax1 = plt.subplot(111)

Then

ax2 = plt.subplot(111, label="label")

will kill ax1, while

ax2 = plt.subplot(111) 

will activate ax1 (hence ax1 == ax2)

@jklymak jklymak added this to the v3.0 milestone Jun 19, 2018
:file:`gallery/pie_and_polar_charts/polar_scatter.py`
For an example
# add a red subplot that share the x-axis with ax1
plt.subplot(224, sharex=ax1, facecolor='red')

Choose a reason for hiding this comment

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

a [..] subplot that shares the ...


**Example:**
#delete x2 from the figure

Choose a reason for hiding this comment

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

delete ax2


.. plot:: gallery/subplots_axes_and_figures/subplot.py
#add x2 to the figure again

Choose a reason for hiding this comment

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

add ax2

@fredrik-1 fredrik-1 force-pushed the doc_subplot_axes branch 4 times, most recently from ffa0750 to 84869a7 Compare June 29, 2018 16:20
@fredrik-1
Copy link
Contributor Author

I have finally made the suggested changes.

@NelleV
Copy link
Member

NelleV commented Jun 29, 2018

@jklymak Because 80 char-wide emacs windows starts wrapping lines at 79 character. (That seems a good reason to be using vim 😇 )

`.Figure` coordinates.

sharex, sharey : `~.axes.Axes`, optional
The x or y `~.matlotlib.axis` is shared with the x or
Copy link
Member

Choose a reason for hiding this comment

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

There's a small typo on this line (matlotlib instead of matplotlib). I'm happy to fix this by pushing directly to your branch.


**kwargs
Other optional keyword arguments:
%(Axes)s
Copy link
Member

Choose a reason for hiding this comment

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

I recall that we had decided to move away from docstring interpolation a couple of years ago. I may be wrong, so ignore this (although it would be good to investigate and document this if it is the case).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know but I suggested to avoid those in another thread some weeks ago but it got turned down (and I have also kind of changed opinion).

def add_axes(self, *args, **kwargs):
"""
Add an axes to the figure.

Call signature::
Call signatures::
Copy link
Member

Choose a reason for hiding this comment

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

You can actually remove those call signatures. We don't use those anymore, as this information is contained in the numpydoc arguments list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that those are sometimes still necessary when there are actually several more or less independent ways to call a function.

Copy link
Member

Choose a reason for hiding this comment

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

Well… it's definitely a sign that the API for those particular functions are way to complicated, but there isn't much we can do about that these days.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. It would also be good to get rid of many of the (*args, **kwargs).

The projection type of the axes.
'polar', 'rectilinear', str}, optional
The projection type of the `~.axes.Axes`. *str* is the name of
a costum projection, see `~matplotlib.projections`. The default
Copy link
Member

Choose a reason for hiding this comment

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

Another small typo here (costum -> custom)

the following table but there might also be other keyword
arguments if another projection is used, see the actual axes
class.
%(Axes)s
Copy link
Member

Choose a reason for hiding this comment

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

ditto about docstring interpolation.


Notes
-----
If the figure already has a axes with key (*args*,
Copy link
Member

Choose a reason for hiding this comment

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

Small typo here as well (a -> an)

def add_subplot(self, *args, **kwargs):
"""
Add a subplot.
Add an `~.axes.Axes` to the figure as part of a subplot arrangement.
Copy link
Member

Choose a reason for hiding this comment

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

👍

@NelleV
Copy link
Member

NelleV commented Jun 29, 2018

@fredrik-1 Thanks a lot for the contribution. I've got a couple of minor comments on the PR, but overall I find that it is an improvement to the documentation, so this is good to be merged, although I have to say I do not feel comfortable with using more docstring interpolation, as we are, to the best of my knowledge, trying to move away from this.

@fredrik-1
Copy link
Contributor Author

I have fixed the last typos.

@NelleV NelleV merged commit 5a22c8b into matplotlib:master Jun 30, 2018
@NelleV
Copy link
Member

NelleV commented Jun 30, 2018

Thanks @fredrik-1 !

@fredrik-1 fredrik-1 deleted the doc_subplot_axes branch July 2, 2018 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants