[MRG] Fixes cross-reference and no reference issues in sphinx by thomasjpfan · Pull Request #13000 · scikit-learn/scikit-learn

thomasjpfan · 2019-01-17T04:19:44Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Overrides the default ReferencesResolver with a CustomReferencesResolver. The custom resolver does two things:

If running in the context of a python class, it places the py domain above the std domain, allowing class methods to be referenced first.
Removes the logging when there is more than one reference.

Any other comments?

This together with #12970 should resolve most of the warnings that appear in sphinx when we switched the default role to "any".

…ross_reference

jnothman · 2019-01-17T04:35:21Z

Hmmm... are you sure this is what you mean? This will override the global default role so that only term and only meth can be respectively linked in these contexts through single backticks without a role being specified (e.g. :meth:). What I think you want is to change the priority in different contexts. That will be done only by redefining the any role, I think.

thomasjpfan · 2019-01-18T03:18:46Z

Yea, I wanted to change the priority order. It looks like the order for 'any' is defined in ReferencesResolver.resolve_anyref. The std domain takes priority.

jnothman · 2019-01-18T04:31:54Z

I'm happy for us to customise the default role but last time I tried to do it it looked too messy so I just went with any

…ross_reference

thomasjpfan · 2019-01-19T17:36:11Z

This PR was updated to override the default ReferencesResolver with a CustomReferencesResolver. The custom resolver does two things:

If running in the context of a python class, it places the py domain above the std domain, allowing class methods to be referenced first.
Removes the logging when there is more than one reference.

Codewise, it adds the following few lines to the default ReferencesResolver.resolve_anyref:

with suppress(KeyError):
    py_domain = self.env.domains['py']
    results.extend(py_domain.resolve_any_xref(
        self.env, refdoc, self.app.builder, target, node, contnode))

and removes the logging that has to do with more than one reference.

thomasjpfan · 2019-01-19T19:26:21Z

This PR was updated with a refactor to clean up the code and make it easier to review. If a reference is found, it will immediately be processed into a node and returned.

jnothman · 2019-01-20T06:53:40Z

Can you help by pointing to links that this improves/changes? Thanks

thomasjpfan · 2019-01-20T12:25:24Z

It prioritizes references in class docstrings to reference itself. Here are a few examples:

SimpleImputer: PR and master with fit and transform.
StandardScaler: PR and master with transform.
LatentDirichletAllocation: PR and master with fit and partial_fit.
GaussianNB: PR and master with partial_fit.

On master the links point to the glossary. With this PR, they point to the class methods.

jnothman · 2019-01-20T20:51:15Z

Looks nice (not looked at code yet)! I have a small concern about referencing base estimator methods in meta-estimator documentation where, if you want a link at all, it is not to the local fit etc, but perhaps to term. But at least there are ways to specify that.

thomasjpfan · 2019-01-20T21:17:21Z

I have a small concern about referencing base estimator methods

This is true, this PR's ColumnTransformer, links fit and transform to its own function, and not the glossary. Since these meta-estimator links are the special case, I think adding term to these references would be preferred.

jnothman · 2019-01-21T00:31:10Z

doc/sphinxext/custom_references_resolver.py

+        target = node['reftarget']
+
+        # first, try resolving as :doc:
+        doc_ref = stddomain.resolve_xref(self.env, refdoc, self.app.builder,


I'm not sure we really ever want to link to :doc:, do we?

jnothman · 2019-01-21T00:32:06Z

doc/sphinxext/custom_references_resolver.py

@@ -0,0 +1,83 @@
+"""Adapted from
+sphinx.transforms.post_transforms.ReferencesResolver.resolve_anyref


Please include any relevant licensing information to avoid breach of terms.

jnothman · 2019-01-21T00:32:21Z

doc/sphinxext/custom_references_resolver.py

+            return self.create_node(('doc', doc_ref))
+
+        # process 'py' domain first for python classes
+        if "py:class" in node:


jnothman · 2019-01-21T00:35:05Z

doc/sphinxext/custom_references_resolver.py

+
+    def create_node(self, result):
+        res_role, newnode = result
+        # Override "any" class with the actual role type to get the styling


Nice!! One confusing thing is that :term: does not generally require monospace, but some terms are explicitly formatted with double backticks in the glossary (but the link target does not include backticks). I assume we're not really able to make that work nicely. No big deal.

I can't see what code controls styling when there's no match.

When the reference is missing the missing-reference event is called. This is handled in another PR: https://github.com/scikit-learn/scikit-learn/pull/12970/files. In that PR, the single backticks are treated as double backticks when there are not matches.

doc/sphinxext/custom_references_resolver.py

thomasjpfan · 2019-01-21T09:49:37Z

This PR was updated to address the "no reference" case. If no reference is found, the single backticked strings will be displayed as <code>. This way, #12970 is no longer needed.

thomasjpfan · 2019-01-21T09:54:29Z

This PR was updated to address the "no reference" case. Codewise, the following was added:

    def resolve_anyref(self, refdoc, node, contnode):
        ...
        # no results considered to be <code>
        contnode['classes'] = []
        return contnode

If no reference is found, the single backticked strings will be displayed as <code>.

jnothman

I like this, but I'm not sure if we should continue calling this 'any'. Is it easy to rename the default role to show maintainers that it's different?

jnothman · 2019-01-21T10:08:43Z

doc/sphinxext/custom_references_resolver.py

+            post_transforms[i] = CustomReferencesResolver
+            break
+    else:
+        raise RuntimeError


Might add a message: ReferencesResolver not found

thomasjpfan · 2019-01-21T14:22:34Z

If we wish to change the default role's name, we would most likely need to create a custom role and a corresponding domain. This would require a different approach than this PR.

A comment was added to the default_role='any' config option to communicate the changes in this PR.

jnothman

Nice work!

thomasjpfan · 2019-01-29T00:53:06Z

@amueller This PR enables the following:

Fixes the "no reference" and cross-reference warnings when the docs are generated.
Configures "single backticks with no reference" to be treated as code blocks (or "double backticks"). - Gives higher priority to Python class methods. If fit can reference a python class method, it will reference that class method instead of the glossary.

adrinjalali · 2019-02-28T18:02:08Z

sklearn/compose/_column_transformer.py

        By setting ``remainder`` to be an estimator, the remaining
        non-specified columns will use the ``remainder`` estimator. The
-        estimator must support `fit` and `transform`.
+        estimator must support :term:`fit` and :term:`transform`.


remove :term: please?

…ross_reference

…earn#13000) * DOC: Fixes cross references * RFC: Minimizes diffs * REV: Removes default-role * ENH: Process python domain first * ENH: Support sphinx 1.6.2 * RFC: Only reorder for python classes * RFC: Adds early exit * DOC: Uses term for ColumnTransformer * REV: Removes styling from term * ENH: Handle no references case * ENH: Adds RuntimeError * RFC: Adds runtime error message * DOC: Adds comment on default_role * CLN Removes term

…cikit-learn#13000)" This reverts commit e52752b.

…earn#13000) * DOC: Fixes cross references * RFC: Minimizes diffs * REV: Removes default-role * ENH: Process python domain first * ENH: Support sphinx 1.6.2 * RFC: Only reorder for python classes * RFC: Adds early exit * DOC: Uses term for ColumnTransformer * REV: Removes styling from term * ENH: Handle no references case * ENH: Adds RuntimeError * RFC: Adds runtime error message * DOC: Adds comment on default_role * CLN Removes term

thomasjpfan added 6 commits January 16, 2019 22:42

DOC: Fixes cross references

fd77398

Merge remote-tracking branch 'upstream/master' into more_one_target_c…

b643c85

…ross_reference

RFC: Minimizes diffs

f59e310

REV

10fc1be

REV

4f8325b

REV

b601666

thomasjpfan mentioned this pull request Jan 17, 2019

[MRG] DOC Avoid missing reference warnings, instead displaying as <code> #12970

Closed

thomasjpfan changed the title ~~[MRG] Fixes cross-reference issues in sphinx~~ [WIP] Fixes cross-reference issues in sphinx Jan 17, 2019

REV: Removes default-role

aa10286

thomasjpfan added 2 commits January 19, 2019 12:29

ENH: Process python domain first

ccfe358

Merge remote-tracking branch 'upstream/master' into more_one_target_c…

122e23e

…ross_reference

thomasjpfan changed the title ~~[WIP] Fixes cross-reference issues in sphinx~~ [MRG] Fixes cross-reference issues in sphinx Jan 19, 2019

thomasjpfan added 5 commits January 19, 2019 12:49

ENH: Support sphinx 1.6.2

43e4052

STY: Flake8

8723924

RFC: Only reorder for python classes

8fadd37

RFC: Only reorder for python classes

b97f737

RFC: Adds early exit

3ebd308

DOC: Uses term for ColumnTransformer

d060281

jnothman reviewed Jan 21, 2019

View reviewed changes

thomasjpfan added 2 commits January 20, 2019 21:56

REV: Removes styling from term

6807f56

ENH: Address comments

79a5316

jnothman approved these changes Jan 21, 2019

View reviewed changes

doc/sphinxext/custom_references_resolver.py Show resolved Hide resolved

ENH: Handle no references case

4f3e5b5

thomasjpfan changed the title ~~[MRG] Fixes cross-reference issues in sphinx~~ [MRG] Fixes cross-reference and no reference issues in sphinx Jan 21, 2019

ENH: Adds RuntimeError

e1b3897

DOC: Better comment

0279fbd

jnothman approved these changes Jan 21, 2019

View reviewed changes

thomasjpfan added 3 commits January 21, 2019 08:46

RFC: Adds runtime error message

828056f

DOC: Adds comment on default_role

29b12c0

DOC: More details

c9e0ab6

jnothman approved these changes Jan 21, 2019

View reviewed changes

glemaitre mentioned this pull request Jan 29, 2019

FIX several bugs in initial predictions for GBDT #12983

Merged

8 tasks

adrinjalali reviewed Feb 28, 2019

View reviewed changes

thomasjpfan added 2 commits February 28, 2019 19:03

Merge remote-tracking branch 'upstream/master' into more_one_target_c…

9fe19bd

…ross_reference

CLN Removes term

b94b598

adrinjalali merged commit db2290c into scikit-learn:master Feb 28, 2019

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

Revert "MNT Fixes cross-reference and no reference issues in sphinx (s…

5a30beb

…cikit-learn#13000)" This reverts commit e52752b.

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

Revert "MNT Fixes cross-reference and no reference issues in sphinx (s…

6fcb161

…cikit-learn#13000)" This reverts commit e52752b.

		@@ -0,0 +1,83 @@
		"""Adapted from
		sphinx.transforms.post_transforms.ReferencesResolver.resolve_anyref

Uh oh!

Conversation

thomasjpfan commented Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

jnothman commented Jan 17, 2019

Uh oh!

thomasjpfan commented Jan 18, 2019

Uh oh!

jnothman commented Jan 18, 2019 via email

Uh oh!

thomasjpfan commented Jan 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomasjpfan commented Jan 19, 2019

Uh oh!

jnothman commented Jan 20, 2019 via email

Uh oh!

thomasjpfan commented Jan 20, 2019

Uh oh!

jnothman commented Jan 20, 2019

Uh oh!

thomasjpfan commented Jan 20, 2019

Uh oh!

jnothman Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

jnothman Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

jnothman Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

jnothman Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

thomasjpfan Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thomasjpfan commented Jan 21, 2019

Uh oh!

thomasjpfan commented Jan 21, 2019

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

jnothman Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

thomasjpfan commented Jan 21, 2019

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

thomasjpfan commented Jan 29, 2019

Uh oh!

adrinjalali Feb 28, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thomasjpfan commented Jan 17, 2019 •

edited

Loading

thomasjpfan commented Jan 19, 2019 •

edited

Loading