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

[REVIEW] Fixing Python Documentation Errors and Warnings #3428

Merged

Conversation

mdemoret-nv
Copy link
Contributor

Closes #3268

General cleanup of sphinx documentation to eliminate all warnings and fix issue #3268. Main change was using autofunction:: instead of automethod:: (which is designed for class methods)

@mdemoret-nv mdemoret-nv added bug Something isn't working 3 - Ready for Review Ready for review by team doc Documentation non-breaking Non-breaking change labels Jan 29, 2021
@mdemoret-nv mdemoret-nv requested a review from a team as a code owner January 29, 2021 03:23
@github-actions github-actions bot added the Cython / Python Cython or Python issue label Jan 29, 2021
Copy link
Member

@dantegd dantegd left a comment

Choose a reason for hiding this comment

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

lgtm

@dantegd dantegd removed the bug Something isn't working label Jan 29, 2021
@dantegd
Copy link
Member

dantegd commented Jan 29, 2021

@gpucibot merge

Copy link
Contributor

@JohnZed JohnZed left a comment

Choose a reason for hiding this comment

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

Looks good - just two questions for my understanding

faster distances calculations
The query algorithm to use. Valid options are:

- ``'brute'``: for brute-force, slow but produces exact results
Copy link
Contributor

Choose a reason for hiding this comment

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

why are the double backticks needed? should all string args be in double backticks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Single and double backticks are shown differently in the documentation. Here is what single backticks look like (italicized, variable spaced):

image

And this is what double backticks look like (bold, monospaced, red):

image

(Depending on where you are viewing the documentation from, there might be slight differences in color, size, etc.)

Generally in Sphinx, you want to use single backticks when you are referring to some other object that exists (i.e. named argument, class, function, etc.). And you want to use double backticks for inline code (which is different than Markdown which everyone is familiar with).

For this particular example, I like to use the double backticks around string literals because: 1) it makes them stand out here in the list, 2) string literals aren't referencing any other object and 3) emphasizes that this code can be copy/pasted if needed.

This really only touches the surface since single backticks are technically "interpreted code" which is domain specific and can be modified with labels. For example, you can cross-reference links in the code by using :ref:`some-other-link` (The :ref: part being a label. See here for the long list of available labels)

@@ -214,7 +216,7 @@ class SVC(SVMBase, ClassifierMixin):
coef_ : float, shape (1, n_cols)
Only available for linear kernels. It is the normal of the
hyperplane.
coef_ = sum_k=1..n_support dual_coef_[k] * support_vectors[k,:]
``coef_ = sum_k=1..n_support dual_coef_[k] * support_vectors[k,:]``
Copy link
Contributor

Choose a reason for hiding this comment

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

ah, so it's double backticks here too -- should every code block in a comment be doubled like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So there were two issues here. If you use a trailing underscore in reST, that means you want to link to an object. So above, coef_ means link to the object named coef. Since no label has been created with the name coef, it generated the following error:

WARNING: Unknown target name: "coef"

Since this section of text was in fact inline code, surrounding it in double backticks correctly displays it as code and also removes the link. So two birds, one stone.

@codecov-io
Copy link

Codecov Report

Merging #3428 (32561ed) into branch-0.18 (550121b) will decrease coverage by 0.08%.
The diff coverage is 85.19%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.18    #3428      +/-   ##
===============================================
- Coverage        71.48%   71.40%   -0.09%     
===============================================
  Files              207      210       +3     
  Lines            16748    16839      +91     
===============================================
+ Hits             11973    12024      +51     
- Misses            4775     4815      +40     
Impacted Files Coverage Δ
python/cuml/datasets/classification.py 80.61% <ø> (ø)
python/cuml/decomposition/incremental_pca.py 94.70% <ø> (ø)
python/cuml/dask/ensemble/base.py 19.69% <30.43%> (+0.36%) ⬆️
python/cuml/dask/cluster/kmeans.py 50.00% <33.33%> (-4.00%) ⬇️
python/cuml/ensemble/randomforestregressor.pyx 70.83% <44.44%> (ø)
python/cuml/dask/decomposition/base.py 36.58% <50.00%> (-2.95%) ⬇️
...ython/cuml/dask/ensemble/randomforestclassifier.py 28.20% <50.00%> (-1.29%) ⬇️
python/cuml/dask/ensemble/randomforestregressor.py 32.72% <50.00%> (-1.82%) ⬇️
python/cuml/dask/linear_model/linear_regression.py 57.14% <50.00%> (-1.95%) ⬇️
python/cuml/dask/linear_model/ridge.py 48.00% <50.00%> (-2.00%) ⬇️
... and 66 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 624555f...32561ed. Read the comment docs.

@rapids-bot rapids-bot bot merged commit 58ed101 into rapidsai:branch-0.18 Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team Cython / Python Cython or Python issue doc Documentation non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DOC] Missing n_samples parameter in datasets.make_classification signature
4 participants