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

silence sphinx warnings round 3 #3602

Merged
merged 32 commits into from
Dec 17, 2019

Conversation

keewis
Copy link
Collaborator

@keewis keewis commented Dec 7, 2019

In this last "sphinx warnings" PR the goal is to silence all nit-picky warnings that are not related to napoleon's interpretation of parameter types. In #3370 (comment) I posted ways to define type aliases (so dict-like points to the term mapping on https://docs.python.org/3/ and array-like to the appropriate page in the numpy docs) or to ignore words like of. This PR applies these to silence all the nit-picky warnings (which mostly means broken links).

As a reference for myself, the documentation of numpydoc's sphinx config options is here: https://numpydoc.readthedocs.io/en/latest/install.html#sphinx-config-options numpydoc does not have anything to do with this, we are blocked by a bug in napoleon (see #3370).

At the moment only the autodoc / autosummary / numpydoc napoleon warnings remain, with a few exceptions in whats-new.rst.

In theory we could also re-enable -n and use the nitpick_ignore settings to ignore any unfixable warnings, but I'm undecided about whether that would be a good idea. Thoughts?

@keewis
Copy link
Collaborator Author

keewis commented Dec 10, 2019

I added a lot of new entries to api-hidden.rst, mostly the methods of CFTimeIndex that were inherited from pandas.Index. Is that okay? sphinx also complains about the attributes (properties?) so I would probably need to add more.

Edit: I also added Data*Coarsen to api-hidden.rst, but we may want to think about giving them the same status as Data*Rolling or Data*Resample: put them into their own section in api.rst.


running sphinx-build with -w warnings and executing grep -e "doc/[^/]*.rst" warnings returns

.../xarray/doc/whats-new.rst:336: WARNING: py:meth reference target not found: Dataset.plot.scatter
.../xarray/doc/whats-new.rst:421: WARNING: py:meth reference target not found: Dataset.plot.scatter
.../xarray/doc/whats-new.rst:468: WARNING: py:class reference target not found: xarray.core.coordinates.DataArrayCoordinates
.../xarray/doc/whats-new.rst:1059: WARNING: py:meth reference target not found: xarray.DataArray.plot.line
.../xarray/doc/whats-new.rst:1091: WARNING: py:meth reference target not found: xarray.tutorial.open_dataset
.../xarray/doc/whats-new.rst:1091: WARNING: py:meth reference target not found: xarray.tutorial.load_dataset
.../xarray/doc/whats-new.rst:1778: WARNING: py:meth reference target not found: DataArray.__dask_scheduler__
.../xarray/doc/whats-new.rst:1780: WARNING: py:meth reference target not found: DataArray.plot.imshow
.../xarray/doc/whats-new.rst:1957: WARNING: py:func reference target not found: xarray.show_versions
.../xarray/doc/whats-new.rst:2029: WARNING: py:meth reference target not found: xarray.backends.PydapDataStore.open

these are the broken links in the non-autodoc / autosummary documentation:

  • if I remember correctly, scatter, line and imshow not having a documentation is known and the link will probably be fixed once we get sphinx to document what I think @dcherian called "injection methods".
  • I'd say __dask_scheduler__ is not part of the public API so we can convert to double-backtick quoted
  • DataArrayCoordinates and DatasetCoordinates still exist, but they are never referenced by the docs. Are they part of the public API?
  • show_versions does not have a docstring (which could be really small) and is not referenced elsewhere in the docs. What should I do with it?
  • the tutorial functions do have docstrings. Should we mention them somewhere else?
  • all the datastore documentation pages have broken links to their methods. Should I also add them to api-hidden.rst?

Copy link
Contributor

@dcherian dcherian left a comment

Choose a reason for hiding this comment

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

Thanks @keewis. This is looking great. I've answered some of your questions below and in the review comments. Thanks again for tackling this.

In theory we could also re-enable -n and use the nitpick_ignore settings to ignore any unfixable warnings, but I'm undecided about whether that would be a good idea. Thoughts?

I'm also unsure on activating -n.

if I remember correctly, scatter, line and imshow not having a documentation is known and the link will probably be fixed once we got sphinx to document what I think @dcherian called "injection method".

I think that's what these are called. Unsure how to make sphinx do the right thing.

I'd say dask_scheduler is not part of the public API so we can convert to double-backtick quoted

👍

DataArrayCoordinates and DatasetCoordinates still exist, but they are never referenced by the docs. Are they part of the public API?

I don't think so.

show_versions does not have a docstring (which could be really small) and is not referenced elsewhere in the docs. What should I do with it?

Let's add a docstring. It's definitely public.

the tutorial functions do have docstrings. Should we mention them somewhere else?

Yes. let's add a tutorial section under API?

all the datastore documentation pages have broken links to their methods. Should I also add them to api-hidden.rst?

Yes these aren't public.

doc/api-hidden.rst Outdated Show resolved Hide resolved
doc/api-hidden.rst Show resolved Hide resolved
doc/api-hidden.rst Show resolved Hide resolved
doc/api-hidden.rst Show resolved Hide resolved
doc/interpolation.rst Show resolved Hide resolved
doc/io.rst Outdated Show resolved Hide resolved
doc/pandas.rst Outdated Show resolved Hide resolved
doc/plotting.rst Outdated Show resolved Hide resolved
doc/terminology.rst Outdated Show resolved Hide resolved
@keewis
Copy link
Collaborator Author

keewis commented Dec 11, 2019

thanks for the review, @dcherian.

The Data*Coordinates objects are returned by the coords attributes of DataArray and Dataset. They are mostly treated as dict but they have a few public (i.e. not prefixed with an underscore) properties (e.g. dims), all of which don't have docstrings. I'll treat both classes as implementation detail.

Other than that, only the injection methods and the datastore methods remain for the manually written part. Unfortunately, the autodoc / autosummary warnings are due to a sphinx bug (see #3370 (comment)), so our only option right now is to use nitpicky_ignore to silence them. I don't think we should, though. Instead, I'll try to silence all warnings that are not due to the bug and we can leave the rest to a potential round 4 or a sphinx / napoleon update.

@dcherian
Copy link
Contributor

The Data*Coordinates objects are returned by the coords attributes of DataArray and Dataset

Ah! I was wrong. These are public and have public methods that should be documented: see https://xarray.pydata.org/en/stable/data-structures.html#coordinates-methods

@dcherian
Copy link
Contributor

Is this ready to merge?

@keewis
Copy link
Collaborator Author

keewis commented Dec 12, 2019

no, not yet. There are a lot of warnings left even if I leave out the ones due to the parameter type.

@keewis
Copy link
Collaborator Author

keewis commented Dec 15, 2019

this is fairly close now:

grep -e "doc/[^/]*.rst" -e "<autosummary>" warnings

gives us

.../xarray/doc/generated/xarray.Dataset.rst:132:<autosummary>:1: WARNING: py:obj reference target not found: xarray.Dataset.dump_to_store
.../xarray/doc/generated/xarray.Dataset.rst:132:<autosummary>:1: WARNING: py:obj reference target not found: xarray.Dataset.load_store
.../xarray/doc/whats-new.rst:346: WARNING: py:meth reference target not found: Dataset.plot.scatter
.../xarray/doc/whats-new.rst:431: WARNING: py:meth reference target not found: Dataset.plot.scatter
.../xarray/doc/whats-new.rst:1068: WARNING: py:meth reference target not found: xarray.DataArray.plot.line
.../xarray/doc/whats-new.rst:1789: WARNING: py:meth reference target not found: DataArray.plot.imshow

which means only the injected methods and the DataStore related Dataset methods are left. If they are still supported, where should I put them?

@keewis
Copy link
Collaborator Author

keewis commented Dec 15, 2019

I think once we decided what to do with the DataStore methods and how to treat the injected methods, we can merge?

Edit: I think we can just leave the accessor methods as is. This means we only have to decide what to do with dump_to_store and load_store. A grep over the code tells me that load_store is never used, tested or mentioned (from_store is mentioned but never defined or used): do we still support it? dump_to_store is mentioned and tested, so we definitely should put it somewhere (even if it is just api-hidden.rst).

@dcherian
Copy link
Contributor

A grep over the code tells me that load_store is never used, tested or mentioned (from_store is mentioned but never defined or used): do we still support it?

I guess from_store mentions should be removed; let's open an issue for removing load_store. I don't know the backend code but it'd gone through multiple refactors so things may have been missed.

dump_to_store is mentioned and tested, so we definitely should put it somewhere (even if it is just api-hidden.rst).

Let's stick this in api-hidden.rst

I think you should merge this after making these changes. It's a large step forward. We can revisit once someone comes up with a solution for the injected methods.

@dcherian dcherian changed the title WIP: silence sphinx warnings round 3 silence sphinx warnings round 3 Dec 17, 2019
@keewis
Copy link
Collaborator Author

keewis commented Dec 17, 2019

I renamed from_store to load_store, let's discuss both in #3638. For the injected / accessor methods there is #3625.

@dcherian
Copy link
Contributor

👍 Wanna merge?

@keewis
Copy link
Collaborator Author

keewis commented Dec 17, 2019

on it

@keewis keewis merged commit 6ad59b9 into pydata:master Dec 17, 2019
@keewis keewis deleted the silence-sphinx-warnings-round-3 branch December 17, 2019 16:25
@dcherian
Copy link
Contributor

👏 Welcome to xarray!

dcherian added a commit to dcherian/xarray that referenced this pull request Dec 30, 2019
* upstream/master:
  added pyinterp to related projects (pydata#3655)
  Allow incomplete hypercubes in combine_by_coords (pydata#3649)
  concat keeps attrs from first variable. (pydata#3637)
  Extend DatetimeAccessor properties and support `.dt` accessor for Timedelta (pydata#3612)
  update readthedocs.yml (pydata#3639)
  silence sphinx warnings round 3 (pydata#3602)
  Fix/quantile wrong errmsg (pydata#3635)
  Provide shape info in shape mismatch error. (pydata#3619)
  Minor doc fixes (pydata#3615)
  Respect user-specified coordinates attribute. (pydata#3487)
  Add Facetgrid.row_labels & Facetgrid.col_labels (pydata#3597)
  Fix pint integration tests (pydata#3600)
  Minor fix to combine_by_coords to allow for the combination of CFTimeIndexes separated by large time intervals (pydata#3543)
dcherian added a commit to dcherian/xarray that referenced this pull request Dec 30, 2019
…oken

* 'master' of github.com:pydata/xarray:
  Add nanmedian for dask arrays (pydata#3604)
  added pyinterp to related projects (pydata#3655)
  Allow incomplete hypercubes in combine_by_coords (pydata#3649)
  concat keeps attrs from first variable. (pydata#3637)
  Extend DatetimeAccessor properties and support `.dt` accessor for Timedelta (pydata#3612)
  update readthedocs.yml (pydata#3639)
  silence sphinx warnings round 3 (pydata#3602)
  Fix/quantile wrong errmsg (pydata#3635)
  Provide shape info in shape mismatch error. (pydata#3619)
  Minor doc fixes (pydata#3615)
  Respect user-specified coordinates attribute. (pydata#3487)
  Add Facetgrid.row_labels & Facetgrid.col_labels (pydata#3597)
  Fix pint integration tests (pydata#3600)
  Minor fix to combine_by_coords to allow for the combination of CFTimeIndexes separated by large time intervals (pydata#3543)
dcherian added a commit to dcherian/xarray that referenced this pull request Dec 30, 2019
…equiv

* 'master' of github.com:pydata/xarray: (28 commits)
  Add nanmedian for dask arrays (pydata#3604)
  added pyinterp to related projects (pydata#3655)
  Allow incomplete hypercubes in combine_by_coords (pydata#3649)
  concat keeps attrs from first variable. (pydata#3637)
  Extend DatetimeAccessor properties and support `.dt` accessor for Timedelta (pydata#3612)
  update readthedocs.yml (pydata#3639)
  silence sphinx warnings round 3 (pydata#3602)
  Fix/quantile wrong errmsg (pydata#3635)
  Provide shape info in shape mismatch error. (pydata#3619)
  Minor doc fixes (pydata#3615)
  Respect user-specified coordinates attribute. (pydata#3487)
  Add Facetgrid.row_labels & Facetgrid.col_labels (pydata#3597)
  Fix pint integration tests (pydata#3600)
  Minor fix to combine_by_coords to allow for the combination of CFTimeIndexes separated by large time intervals (pydata#3543)
  Fix map_blocks HLG layering (pydata#3598)
  Silence sphinx warnings: Round 2 (pydata#3592)
  2x~5x speed up for isel() in most cases (pydata#3533)
  remove xarray again (pydata#3591)
  fix plotting with transposed nondim coords. (pydata#3441)
  make coarsen reductions consistent with reductions on other classes (pydata#3500)
  ...
dcherian added a commit to dcherian/xarray that referenced this pull request Dec 30, 2019
* upstream/master:
  Add nanmedian for dask arrays (pydata#3604)
  added pyinterp to related projects (pydata#3655)
  Allow incomplete hypercubes in combine_by_coords (pydata#3649)
  concat keeps attrs from first variable. (pydata#3637)
  Extend DatetimeAccessor properties and support `.dt` accessor for Timedelta (pydata#3612)
  update readthedocs.yml (pydata#3639)
  silence sphinx warnings round 3 (pydata#3602)
  Fix/quantile wrong errmsg (pydata#3635)
  Provide shape info in shape mismatch error. (pydata#3619)
  Minor doc fixes (pydata#3615)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hundreds of Sphinx errors
2 participants