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] Cleanup sphinx doc warnings for 0.15 #2649

Merged

Conversation

mdemoret-nv
Copy link
Contributor

This PR pulls warnings cleanup from mdemoret-nv:fea-improve-doc-examples-source which was completed as part of the documentation source improvement. Since that PR is blocked until 0.16, the warning cleanup was cherry picked into a separate PR.

While there are still outstanding warnings, this does clean up the majority of the basic warnings and improves the documentation. This includes the following types of fixes:

  • Normalization of section headers
  • Fixing References styling to be consistent and look different from attributes/parameters
  • Distinction between single backticks, used for referencing members/functions/classes/modules, and double backticks, for inline code
  • Addition of .. note:: sections to replace "Note: "
  • Cleanup of indentation errors common with lists and parameters
  • Addition of \ where needed for parameter types
  • General fixing of indentation and newlines

Remaining warnings that were not cleaned up:

  1. Uncommon section names (such as Known Limitations, Performance, Tips, etc.). These require input from the author on the correct section to move them into.
  2. Autosummary failed imports. It's unclear why this is happening.
  3. Odd, WARNING: Unexpected indentation. messages. The cause is unclear for these warnings.
  4. WARNING: duplicate object description... seem to be caused by very similar documentation for different objects. Need author input on whether the docs can be tweaked to show the difference between the objects.
  5. WARNING: Unknown target name. The following link suggests this may be due to hyperlinks, however removing all links did not cause the messages to disappear. Need further research.

@dantegd This should hopefully work well with #2635. I would appreciate any feedback you have on these changes

@mdemoret-nv mdemoret-nv requested a review from a team as a code owner August 7, 2020 06:01
@GPUtester
Copy link
Contributor

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

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 pending CI and changelog conflict fix, found one small character that might not be needed but besides that its great to see this fixes!

@@ -95,7 +98,7 @@ def __init__(self, model, client=None, **kwargs):
self._set_internal_model(model)

def transform(self, X, convert_dtype=True):
"""
r"""
Copy link
Member

Choose a reason for hiding this comment

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

why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I was seeing some weird errors with hyperlinks and tried to convert to a literal string to avoid any html link issues. It did not work so this can be removed.

@mdemoret-nv
Copy link
Contributor Author

The latest commits have cleaned up all possible sphinx warnings and errors. To resolve them, the following changes were made (numbers match the remaining warnings section above):

  1. Uncommon section names (such as Known Limitations, Performance, Tips, etc.) have either been moved into the Notes section, or converted to inline Admonitions (i.e. tips, notes, warnings, etc.)
  2. Autosummary import was resolved by giving the full path
  3. Unexpected indentations were resolved by removing trailing _ that were not necessary
  4. Both LabelEncoder and LabelBinarizer were included in the docs twice. A :noindex: directive cleaned up the warning
  5. Unknown target name and unexpected indentation were both caused by the same issue.

@dantegd Can you give this a quick look and merge once CI has completed?

@dantegd dantegd added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Aug 12, 2020
@dantegd dantegd merged commit 141d7c9 into rapidsai:branch-0.15 Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants