Skip to content

Doc warnings #14699

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Oct 18, 2019
Merged

Doc warnings #14699

merged 8 commits into from
Oct 18, 2019

Conversation

mattip
Copy link
Member

@mattip mattip commented Oct 15, 2019

Solve some of the WARNINGS when building documentation with SPHINXOPTS=-n make html. xref gh-13114. Here is how I found and fixed these:

source <path-to-python3-virt>/bin/activate
cd doc # into the numpy documentation directory
(cd ..; pip install .) # remakes and installs numpy into the virtual environment
make clean
SPHINXOPTS=-nq make html &2 > 1 |tee /tmp/shpinx_build.html
cut -f4-7 -d: /tmp/sphinx_build.txt |sort | uniq -c | sort -n

The last line sorts and prints out the frequency of the WARNINGS. It seems sphinx is limited to about 1000 WARNINGS. We had over that limit, after this PR it is down to about 600.

In order to detect what the reference should be, I use this to search the inverse index of references, i.e for ndarray.all:

$ tail -n +5 build/html/objects.inv | zlib-flate -uncompress |grep -i ndarray.all

which prints

numpy.ndarray.all py:method 1 reference/generated/numpy.ndarray.all.html#$ -
reference/generated/numpy.ndarray.all std:doc -1 \
     reference/generated/numpy.ndarray.all.html numpy.ndarray.all

The first line is the important one: numpy.ndarray.all is a py:method reference, so using ndarray.all is not sufficient, either use the full name or add a .. currentmodule:: numpy directive. I chose to add the explicit namespace to many of the "See Also" references since I think it looks better when using console-based help(numpy.any)

rgommers
rgommers reviewed Oct 15, 2019
View reviewed changes
doc/source/reference/arrays.dtypes.rst Outdated
@@ -128,7 +128,7 @@ What can be converted to a data-type object is described below:

Used as-is.

:const:`None`
`None`
Copy link
Member

Choose a reason for hiding this comment

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

I think you want double backticks or no backticks for all of these. Not sure if this turns it into a link, but that's weird anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the source as to why :const: is wrong - it should be :data:. In any case, sphinx makes this a link with no directive needed

Copy link
Member

Choose a reason for hiding this comment

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

To be clear, what I'm saying is: the normal thing to do is to not make None a link at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

git grep "\`None\`"

turns up many more cases of this. Should the default be not to link?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I'd say so. I remember a discussion about this when we first drew up the docstring standard.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

rgommers
rgommers reviewed Oct 15, 2019
View reviewed changes
numpy/core/_add_newdocs.py
linspace : Evenly spaced numbers with careful handling of endpoints.
ogrid: Arrays of evenly spaced numbers in N-dimensions.
mgrid: Grid-shaped arrays of evenly spaced numbers in N-dimensions.
numpy.linspace : Evenly spaced numbers with careful handling of endpoints.
Copy link
Member

Choose a reason for hiding this comment

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

This is weird. I think this fix, and the many like it, are indicating a bug crept into numpydoc or add_newdoc. This used to work, and while these changes fix warnings I'd prefer to solve the real issue instead.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it has to do with the place argument to add_newdoc. For example for this function, it uses add_newdoc('numpy.core.multiarray', 'arange', ...). Would the issue be fixed if using add_newdoc('numpy', 'arange', ...) instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would call this a feature, not a bug. Explicit beats implicit, especially with documentation. For the on-line documentation the extra 6 letters don't matter, they are turned into a link. For the off-line documentation, a bare linspace can be confusing, especially when just getting started.

Copy link
Member

Choose a reason for hiding this comment

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

This works in many places in the same file without the prefix. Example: https://numpy.org/devdocs/reference/generated/numpy.array.html#numpy.array

If there's a good reason it needs to be this way I have no problem with adding six characters, but I want to understand why something that used to work reliably (I think) now sometimes fails.

Copy link
Member Author

Choose a reason for hiding this comment

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

Something is off. The rendered documentation for this fix is creating the links even without it. Maybe I overdid it with adding numpy. where not needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I rebased without this commit, will look at it again later

@rgommers
Copy link
Member

The first line is the important one: numpy.ndarray.all is a py:method reference, so using ndarray.all is not sufficient, either use the full name or add a .. currentmodule:: numpy directive.

For methods I think this sounds right. For functions however it doesn't, something like `linspace` shouldn't need to be prefixed with numpy.

@mattip
Copy link
Member Author

mattip commented Oct 15, 2019

The last commit gets us down to 859 warnings, which means they all are displayed (less than the 1000 limit). This was my original goal; in PR gh-14608 there were bad references I missed since the warnings were not shown.

@bsipocz
Copy link
Member

bsipocz commented Oct 15, 2019

For methods I think this sounds right. For functions however it doesn't, something like linspace shouldn't need to be prefixed with numpy.

My understanding is that the short versions only works when it's defined in the same place, or at least available in the namespace of the given file.
The API link in fact resolves to the full path, so it's probably better practice to spell out the full link.

This was referenced Oct 16, 2019
Merged
Merged
@rgommers rgommers merged commit 143d713 into numpy:master Oct 18, 2019
@rgommers
Copy link
Member

Okay this is a major improvement in the doc build status, so let's get it in and worry about the prefix thing later.

Merged, thanks @mattip

@rgommers rgommers added this to the 1.18.0 release milestone Oct 18, 2019
@mattip mattip mentioned this pull request Oct 20, 2019
Merged
@mattip mattip deleted the doc-warnings branch October 13, 2020 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgommers rgommers rgommers left review comments

Assignees
No one assigned
Labels
04 - Documentation
Projects
None yet
Milestone
1.18.0 release
Development

Successfully merging this pull request may close these issues.
3 participants
@mattip @rgommers @bsipocz