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: document subclasses in API docs with selection of specific methods/attributes #18202

Merged
merged 34 commits into from
Nov 15, 2017

Conversation

TomAugspurger
Copy link
Contributor

xref #18147

OK, 500ab71 removes our hack in the vendored numpydoc/numpydoc.py to exclude autosummary generation for certain methods. Instead, we'll explicitly list the relevant methods in a new Methods section in those docs. This let's us include things like IntervalIndex in the API docs, without having all the regular index methods in the table, and without auto-gen making pages for all of those.

The downside is that we have to list the methods in Methods in the docstring and under a toctree in api.rst. But that's not so bad.

257fc8a is removing all the warnings about references a non-existent document / this document isn't included in a toctree. But it's a bit tedious so I'll come back later (~600 warnings remaining :/ )

@@ -845,12 +863,16 @@ Binary operator functions

DataFrame.add
DataFrame.sub
DataFrame.subtract
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thoughts on these aliases? IMO, since there methods we should document them. But I can also put them in our "hidden" block down below.

Copy link
Member

Choose a reason for hiding this comment

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

We should document them in the sense that they have a docstring and will end up in the class docstring page of DataFrame in the full list of members, but I don't think that should mean we should include them all in this place.

@@ -692,6 +698,10 @@ adding ordering information or special categories is need at creation time of th
.. autosummary::
:toctree: generated/

Categorical.categories
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 know that this is the wrong place for this (doesn't fit w/ the text above). I'm going to make a dedicated section for Categorical.

@TomAugspurger
Copy link
Contributor Author

cc @jorisvandenbossche

----------
codes
categories
ordered
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we would add here a sentence with something like "For all other methods, see Index", but I am not sure that is allowed in Attributes / Methods sections?

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 suspect not. I've been thinking of ways to handle this.

I'm not worried about people reading the source / in IPython. They already have the code or object.

I think in the Notes section we can say "See :class:Index for additional methods." or something like that.

@jorisvandenbossche
Copy link
Member

The downside is that we have to list the methods in Methods in the docstring and under a toctree in api.rst. But that's not so bad.

While looking at the autodoc code, I was thinking of a possible different way that would not need this listing in both places. There is a autodoc-skip-member event emitted, where we might be able to skip certain members depending on the class (but not sure this would actually work for our use case, or would actually be better)

257fc8a is removing all the warnings about references a non-existent document / this document isn't included in a toctree. But it's a bit tedious so I'll come back later (~600 warnings remaining :/ )

Are you sure this is needed? As it is currently not needed to not get the warnings (I see you set numpydoc_show_class_members to False, is that related?) And is this independent from the manual Methods listing, or is this a consequence of trying to fix that?

@codecov
Copy link

codecov bot commented Nov 10, 2017

Codecov Report

Merging #18202 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18202      +/-   ##
==========================================
- Coverage    91.4%   91.35%   -0.05%     
==========================================
  Files         164      164              
  Lines       49880    49854      -26     
==========================================
- Hits        45591    45545      -46     
- Misses       4289     4309      +20
Flag Coverage Δ
#multiple 89.16% <ø> (-0.03%) ⬇️
#single 39.44% <ø> (-0.04%) ⬇️
Impacted Files Coverage Δ
pandas/plotting/_core.py 82.45% <ø> (ø) ⬆️
pandas/core/tools/datetimes.py 84.48% <ø> (ø) ⬆️
pandas/core/reshape/reshape.py 100% <ø> (ø) ⬆️
pandas/io/formats/style.py 100% <ø> (ø) ⬆️
pandas/core/indexes/category.py 97.46% <ø> (ø) ⬆️
pandas/io/gbq.py 25% <ø> (-58.34%) ⬇️
pandas/core/categorical.py 95.75% <ø> (ø) ⬆️
pandas/core/indexes/period.py 92.9% <ø> (ø) ⬆️
pandas/core/indexes/timedeltas.py 91.14% <ø> (ø) ⬆️
pandas/core/indexes/multi.py 96.38% <ø> (ø) ⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63e8527...34834fa. Read the comment docs.

@TomAugspurger
Copy link
Contributor Author

Sorry I made a mistake earlier w/ the warnings. Most are coming from me forgetting to use the class_without_autosummary template.

I'm not sure why things like DataFrame.as_blocks are causing warning now.

@jreback jreback added the Docs label Nov 10, 2017
@TomAugspurger
Copy link
Contributor Author

Doing a clean build again. Some things I would appreciate feedback on:

  1. I was probably inconsistent with where I put these methods (either sections or in the 'hidden' section at the bottom)
  2. Do we want to setup a linter on the sphinx build so that CI fails when there are any warnings?
  3. Methods like .mul and .multiply: do we want to list both?

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

In the rendered output it is always first attributes and then methods, so maybe we should keep the same order in the docstrings?

Series.imag
Series.real
Series.name
Series.put
Copy link
Member

Choose a reason for hiding this comment

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

I have the feeling that we don't want to include all of those here (eg imag, real). If that is needed for getting rid of warnings, we can put them in comment?)

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 didn't look closely in this last round. Happy to move them to the bottom hidden section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And yes, it's for warnings. Every method of a class that uses the default class template (with autosummary) will need to be listed in a toctree. That's why I've added the :hidden: toctree at the bottom. I'll also update contributing.rst with some details.

@@ -845,12 +863,16 @@ Binary operator functions

DataFrame.add
DataFrame.sub
DataFrame.subtract
Copy link
Member

Choose a reason for hiding this comment

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

We should document them in the sense that they have a docstring and will end up in the class docstring page of DataFrame in the full list of members, but I don't think that should mean we should include them all in this place.

@@ -67,6 +67,7 @@
]

exclude_patterns = ['**.ipynb_checkpoints']
numpydoc_show_class_members = False
Copy link
Member

Choose a reason for hiding this comment

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

Do you know why this is needed?

@jorisvandenbossche
Copy link
Member

How do the rendered docstring pages look like of those classes where you included it manually?

@TomAugspurger
Copy link
Contributor Author

attributes and then methods

Yes, I've been inconsistent. I'll fix that. There's a numpydoc issue talking about emitting warnings if the sections aren't in the correct order.

How do the rendered docstring pages look like of those classes where you included it manually?

I think everything looks correct:

screen shot 2017-11-12 at 10 39 03 am

@TomAugspurger
Copy link
Contributor Author

FYI: the warnings are now down to the cyfunc ones and a whole bunch of new ones from the offsets :) I'm going to think about the offsets ones, they aren't super useful in their current state.

@jorisvandenbossche
Copy link
Member

And the docstring pages for those that should have all automatically like DataFrame, they still list them all?

@jreback
Copy link
Contributor

jreback commented Nov 12, 2017

I'm going to think about the offsets ones, they aren't super useful in their current state.

mea culpa. upside is I have a PR with some doc-strings :>

@jorisvandenbossche
Copy link
Member

FYI: the warnings are now down to the cyfunc ones and a whole bunch of new ones from the offsets :) I'm going to think about the offsets ones, they aren't super useful in their current state.

I would personally be fine with removing them again temporary if that makes us to go faster towards fully working docs (I don't know how much work it is to fix them), to achieve a warning free build, and then once everything is in place we can add them properly.

@jreback
Copy link
Contributor

jreback commented Nov 12, 2017

I would personally be fine with removing them again temporary if that makes us to go faster towards fully working docs (I don't know how much work it is to fix them), to achieve a warning free build, and then once everything is in place we can add them properly.

ok too. I really just needed to_offset.

@TomAugspurger
Copy link
Contributor Author

And the docstring pages for those that should have all automatically like DataFrame, they still list them all?

Ah, no! I'm rebuilding with show_class_members = True, which I hope will fix that. I don't recall why I set that to False a while back.

@TomAugspurger
Copy link
Contributor Author

mea culpa. upside is I have a PR with some doc-strings :>

:) No worries. I was a bit surprised when I thought I had all the warnings squashed and it jumped to 200+ again :)

@TomAugspurger
Copy link
Contributor Author

I'm rebuilding with show_class_members = True, which I hope will fix that. I don't recall why I set that to False a while back.

Hmm apparently that's necessary to make the methods section work? I now see warnings like

/Users/taugspurger/Envs/pandas-dev/lib/python3.6/site-packages/pandas/doc/source/generated/pandas.Categorical.rst:101: WARNING: toctree references unknown document 'generated/pandas.Categorical.add_categories'

and

<autosummary>:None: WARNING: toctree contains reference to nonexisting document 'generated/pandas.Grouper.ax'

and

None:None: WARNING: toctree contains reference to nonexisting document 'generated/pandas.Categorical.add_categories'

2373 warnings in total. Everything seems to render correctly though...

@TomAugspurger
Copy link
Contributor Author

and so it should not be necessary to add all methods in api.rst ?

I'll double check, but the warnings I was getting was like "this pandas.Series.hasnans.rst file is not included in any toctree" since it was autogenerated, but not included in api.rst. I think that's the ideal behavior? We want a warning when we've forgotten to include it (potentially in the :hidden: toctree).

@jorisvandenbossche
Copy link
Member

Hasnans is an attribute. Did you also get the same for warnings for methods? I also had problems with attributes, but think this is a bug / related to the attributes table formatting issue

@TomAugspurger
Copy link
Contributor Author

TomAugspurger commented Nov 14, 2017

Hasnans is an attribute. Did you also get the same for warnings for methods?

@jorisvandenbossche just tested again and you're correct. It is just unlisted attributes that emit a warning on numpydoc master. Methods do not.

@jorisvandenbossche
Copy link
Member

OK, that mystery is solved then :-) So the question now is: given you did the work, do we just keep them in there? (all the hidden ones)

@TomAugspurger
Copy link
Contributor Author

Those last two commits should do it, although I get a bizarre warning for Float64Index:

/Users/taugspurger/Envs/pandas-dev/lib/python3.6/site-packages/pandas/pandas/core/indexes/numeric.py:docstring of pandas.Float64Index:42: WARNING: Inline literal start-string without end-string.

But not any of the other numeric indexes.

@TomAugspurger
Copy link
Contributor Author

Merging later today if there are no other comments.

@jorisvandenbossche
Copy link
Member

I just did a local build with this branch, it is looking fine! (using numpydoc master as well, but with sphinx 1.5)

I just got a big bunch of warnings on Timestamp.fold not being importable / toctree referencing nonexisting document, because this is new in py3.6 (my dev env is still 3.5). So that will be the case for everybody building on py < 3.6.

Further question, just for my understanding: do you know why numpydoc, for the class docstring pages where we manually listed the attributes/methods, it did not add the rest in that list as well (as it seems this is what is happening in eg scipy (https://docs.scipy.org/doc/scipy/reference/generated/scipy.spatial.Delaunay.html#scipy.spatial.Delaunay was given as example in the numpydoc issue. There the manually specified and automatically generated autosummary table seems to be fused / numpy/numpydoc#106 (comment))

@jorisvandenbossche jorisvandenbossche changed the title [WIP]: API doc methods DOC: document subclasses in API docs with selection of specific methods/attributes Nov 15, 2017
@jorisvandenbossche jorisvandenbossche merged commit c2590b3 into pandas-dev:master Nov 15, 2017
@jorisvandenbossche
Copy link
Member

@TomAugspurger Thanks a lot for this!

@jorisvandenbossche
Copy link
Member

@TomAugspurger Do we try to include this for 0.21.1, or leave for 0.22? (I am not sure how cleanly it will backport, and then also the related PRs should be backported)

@jreback
Copy link
Contributor

jreback commented Nov 15, 2017

it would be nice to backport but might be some work

@TomAugspurger TomAugspurger deleted the api-doc-methods branch December 20, 2017 16:11
gfyoung added a commit to forking-repos/pandas that referenced this pull request Oct 28, 2018
Was dropped in pandas-devgh-15099 but accidentally added back in pandas-devgh-18202.

Follow-up to pandas-devgh-23375.
jreback pushed a commit that referenced this pull request Oct 28, 2018
Was dropped in gh-15099 but accidentally added back in gh-18202.

Follow-up to gh-23375.
tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request Nov 19, 2018
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants