ENH: Xref param type

[larsoner]

Takes over and closes #150 with:

• rebase
• fixing tests
• changing into opt-in behavior
• fix rendering bugsRendering bugs were a sphinx issue BUG: numpydoc + autosumarry broken sphinx-doc/sphinx#5959
• add hint about CSS styles for uniformity.

[larsoner]

• I didn't like that having the alias:

  'SourceSpaces': '~mne.SourceSpaces`
  

  did not produce the same style as the more explicit but expected to be equivalent

  'SourceSpaces': ':class:`SourceSpaces <mne.SourceSpaces>`'
  

  So I tweaked the xref node to use the literal text type rather than Text.

• Then I didn't like that the :ref: links in parameter lists (which I used for bool and matplotlib colormap) had a different style than the :class: links, so I added hint to install.rst docs about how to deal with it in CSS. (I don't think there is any Sphinx mechanic to make them uniform, because :ref: targets are not meant to render the same way as :class: ones.)

• I still don't like that I need to provide an alias for SourceSpaces at all, given that it exists in the root of the module I'm documenting, and looking at the current module in env in the xref node type, it's even set properly. But I couldn't figure out how to get it to automatically find the correct one, even after attaching the env information how I think they do it in Sphinx. But perhaps this is not a blocker, since using .SourceSpaces would have worked (what @has2k1 probably did?) and this can always be added later.

@has2k1 can you try my branch and make sure it looks okay on the repo(s) you've tested your branch on?

In MNE it took about 100 lines of custom defs to get it working (and a lot of sloppy docstring unifying) without warnings in nitpicky mode, see here. This was a lot of work because we have a lot of functions, but I think it's acceptable.

@jnothman if you are feeling brave, you are welcome to try it for sklearn. Although it might be even more important there to have the currentmodule directive respected. If you have any ideas on how to do it, it would be much appreciated!

[larsoner]

@jnothman this could be a cool thing to try if at some point there is a sklearn sprint going on :)

For how I got it working in MNE feel free to look at this PR

[jnothman]

Thanks @larsoner.... I think this is something to try outside of the sprint. We've got lots of old issues to resolve this week.

[rgommers]

@larsoner is this still WIP?

[larsoner]

I have some caveats above but it seems to work well for MNE when I use it. The remaining sticking point is that I had to manually provide a bunch of link names, which was annoying. But if that's not a blocker for people then yes it's ready for review/merge.

There is a merge conflict so I'll rebase.

[larsoner]

Perhaps this should wait until @jnothman or someone who knows a bit more about the docutils internals can see if there is a way to automatically get the classes that are currently being documented to link properly. It would increase usability quite a bit, especially for larger libraries like scikit-learn and Pandas.

Also @FHaase I noticed you tried #150 a few months back in pandas-dev/pandas#24098, feel free to try this if you want.

[jnothman]

automatically get the classes that are currently being documented to link properly

Sorry it's been a while since I've tried to consider this. What do you mean here?

[larsoner]

Here is an example: in MNE there are objects such as mne.SourceEstimate that our default_role='autolink' sphinx config will automatically find the right name for in docstrings even when we use a short version, such as:

... use a `SourceEstimate` to ...

On this PR, in parameter lists the following would link just fine:

stc : instance of mne.SourceEstimate

But this will not:

stc : instance of SourceEstimate

despite the fact that the autolink default role is smart enough to resolve what a SourceEstimate ref is.

Instead, the solution I've been using is an alias list numpydoc_xref_aliases, that e.g. converts any SourceEstimate in param lists into ~mne.SourceEstimate. Some of these aliases are probably necessary (for example that when we say color we mean matplotlib:api/colors_api), but some of them seem like there must be a way to be more clever in code to avoid needing them. But I could not figure out how to automatically get SourceEstimate to resolve to the correct name in the same way that the autolink role does it.

You can imagine that building this list would probably be worse for scikit-learn and larger libs than it was for MNE.

There is also an ignore list numpydoc_xref_ignore that is necessary if you want to avoid warnings about missing links. I think it would perhaps be better if we didn't warn about these, but this list is much less annoying to construct than many of the the seemingly unnecessary entries in the alias list.

[larsoner]

Part of the problem (maybe all of it?) might have to do with not making use of the currentmodule, too. SourceEstimate should resolve to mne.SourceEstimate when the currentmodule is mne, but it doesn't.

Similarly, when documenting e.g. mne.preprocessing.run_ica, having this in the non-param-list part of the docstring works just fine:

... look at the :class:`ICA` object ...

because the .. currentmodule at that point is mne.preprocessing. But in the parameter list, none of the following currently autolink to mne.preprocessing.ICA:

`ICA`, :class:`ICA`, `preprocessing.ICA`, :class:`preprocessing.ICA`

But they all probably should. Instead, the explicit link I point to above must be supplied:

'ICA': '~mne.preprocessing.ICA'

[jnothman]

Right, yes, I can imagine that resolution would be very difficult at numpydoc munging time. It's possible @thomasjpfan has ideas about referencing things with respect to the current module.

Remind me: What's the problem with always just using the default role (or a specified role) for words not on a blacklist?

[larsoner]

I think the original idea was to avoid warnings about missing/bad links. (I didn't write this bit of code.) It might indeed be simpler to just wrap in backticks, maybe with some default role (maybe configurable).

[larsoner]

• Rebased
• Changed it just to use :obj:
• Added some presumably very common default values for Python and NumPy aliases to make adoption easier
• Clarified configuration options
• Cleaned up tests

In MNE we still need about 80 lines of class definitions, but I noticed that these are actually the same sorts of definitions that are necessary if you manually put the refs in the docstring, so I think it's probably okay at this point.

I think this one is finally ready for review/merge from my end.

[has2k1]

@larsoner, thank you for picking this up. I will also tryout your updates on my packages. I have been using this feature for the plotnine API documentation for over a year now.

[rgommers]

@larsoner this needs a rebase now

[larsoner]

Rebased and Travis is happy.

[rgommers]

@jnothman are you okay with merging this?

[larsoner]

@jnothman feel free to look at my sklearn branch where I added a commit to get things moving. This is the result for PCA:

I can open a WIP PR to sklearn using this branch if you want to see a full build, but figured I'd check with you before doing so since I know sklearn gets tons of PRs. You can also check out my branch and build locally.

[has2k1]

I have updated the demo page to use this PR.

+1 on merging.

[jnothman]

As long as it's easy to opt out I'm happy to see this available. I sort of wish it had more reuse of the Sphinx role functionality but that can be a nuisance to customise anyway. Sorry I'm not available to review in detail right now

[rgommers]

This looks good to go, let's get it in. Thanks @has2k1 for the nice enhancement, and @larsoner for finishing off the PR!

As long as it's easy to opt out

It's opt-in, so no change by default.

[larsoner]

For sklearn and packages with a good default role it might be good to allow specifying the role prefix. Right now it's :obj: but that could easily be customizable to be an empty string to use the default one