search

scikit-learn / enhancement_proposals

Enhancement proposals for scikit-learn: structured discussions and rational for large additions and modifications
https://scikit-learn-enhancement-proposals.readthedocs.io/en/latest
BSD 3-Clause "New" or "Revised" License
48 stars 34 forks source link

[MRG] Sphinx doc #9

Closed NicolasHug closed 5 years ago

NicolasHug commented 5 years ago

I pulled all 4 current PRs and put them together with sphinx (RTD theme).

I can spin up a RTD project as well when / if this is merged.

NicolasHug commented 5 years ago

screenshot_2018-12-12 scikit-learn enhancement proposals scikit-learn enhancement proposals documentation

amueller commented 5 years ago

Awesome!

Oh can you also add a template based on the NEP template? @GaelVaroquaux said (in a super secret discussion)

I like the NEP template a lot, but I would like to change a couple of things to it: I would like to put more importance to the use case; also, I would like to add a section on validation. But I think that starting from the NEP and modifying it a little is the right way.

I think for a first draft just copy/pasting the template and replacing NEP by SLEP or something should be fine. If there's obvious ways to prioritize use-cases we can maybe do that now.

amueller commented 5 years ago

Can you also make sure that intersphinx works and links to the relevant docs?

NicolasHug commented 5 years ago

It works, but we have to use the full syntax i.e. 

:class:`sklearn.pipeline.Pipeline`

The "easy" version with simple backticks does not work.

amueller commented 5 years ago

Did you use the same default role as in the sklearn repo?

NicolasHug commented 5 years ago

Oh yeah it works with default_role = 'any'

but now there's a lot of warnings

``` /home/nico/dev/enhancement_proposals/slep001/proposal.rst:154: WARNING: 'any' reference target not found: pipeline.named_steps['lasso'].coef_ /home/nico/dev/enhancement_proposals/slep001/proposal.rst:158: WARNING: 'any' reference target not found: pipeline.steps[-1][1].coef_ /home/nico/dev/enhancement_proposals/slep001/proposal.rst:226: WARNING: 'any' reference target not found: TransModifier /home/nico/dev/enhancement_proposals/slep001/proposal.rst:229: WARNING: 'any' reference target not found: X_new, y_new = estimator.fit_modify(X, y) /home/nico/dev/enhancement_proposals/slep001/proposal.rst:231: WARNING: 'any' reference target not found: X_new, y_new = estimator.trans_modify(X, y) /home/nico/dev/enhancement_proposals/slep001/proposal.rst:235: WARNING: 'any' reference target not found: X_new, y_new, sample_props = estimator.fit_modify(X, y) /home/nico/dev/enhancement_proposals/slep001/proposal.rst:237: WARNING: 'any' reference target not found: X_new, y_new, sample_props = estimator.trans_modify(X, y) /home/nico/dev/enhancement_proposals/slep001/proposal.rst:241: WARNING: 'any' reference target not found: fit_modify /home/nico/dev/enhancement_proposals/slep001/proposal.rst:241: WARNING: 'any' reference target not found: trans_modify /home/nico/dev/enhancement_proposals/slep001/proposal.rst:244: WARNING: 'any' reference target not found: fit_modify /home/nico/dev/enhancement_proposals/slep001/proposal.rst:263: WARNING: 'any' reference target not found: trans_modify /home/nico/dev/enhancement_proposals/slep001/proposal.rst:272: WARNING: 'any' reference target not found: cross_val_score /home/nico/dev/enhancement_proposals/slep001/proposal.rst:272: WARNING: 'any' reference target not found: GridSearchCV /home/nico/dev/enhancement_proposals/slep001/proposal.rst:290: WARNING: 'any' reference target not found: trans_modify /home/nico/dev/enhancement_proposals/slep001/proposal.rst:295: WARNING: 'any' reference target not found: trans_modify /home/nico/dev/enhancement_proposals/slep001/proposal.rst:295: WARNING: 'any' reference target not found: y_true /home/nico/dev/enhancement_proposals/slep001/proposal.rst:312: WARNING: 'any' reference target not found: sample_props /home/nico/dev/enhancement_proposals/slep001/proposal.rst:315: WARNING: 'any' reference target not found: sample_props /home/nico/dev/enhancement_proposals/slep002/proposal.rst:91: WARNING: 'any' reference target not found: collections.OrderedDict /home/nico/dev/enhancement_proposals/slep002/proposal.rst:106: WARNING: 'any' reference target not found: steps /home/nico/dev/enhancement_proposals/slep002/proposal.rst:106: WARNING: 'any' reference target not found: Pipeline.steps /home/nico/dev/enhancement_proposals/slep002/proposal.rst:106: WARNING: 'any' reference target not found: list /home/nico/dev/enhancement_proposals/slep002/proposal.rst:106: WARNING: 'any' reference target not found: steps /home/nico/dev/enhancement_proposals/slep002/proposal.rst:106: WARNING: 'any' reference target not found: list /home/nico/dev/enhancement_proposals/slep002/proposal.rst:122: WARNING: 'any' reference target not found: None /home/nico/dev/enhancement_proposals/slep002/proposal.rst:131: WARNING: 'any' reference target not found: add /home/nico/dev/enhancement_proposals/slep002/proposal.rst:159: WARNING: 'any' reference target not found: (step, estimator) /home/nico/dev/enhancement_proposals/slep002/proposal.rst:197: WARNING: 'any' reference target not found: .steps /home/nico/dev/enhancement_proposals/slep002/proposal.rst:211: WARNING: 'any' reference target not found: replace() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:220: WARNING: 'any' reference target not found: rename() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:230: WARNING: 'any' reference target not found: pipeline.set_params() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:230: WARNING: 'any' reference target not found: subestimator.set_params() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:246: WARNING: 'any' reference target not found: del /home/nico/dev/enhancement_proposals/slep002/proposal.rst:266: WARNING: 'any' reference target not found: pop() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:285: WARNING: 'any' reference target not found: pop(step_name) /home/nico/dev/enhancement_proposals/slep002/proposal.rst:285: WARNING: 'any' reference target not found: popitem(step_name) /home/nico/dev/enhancement_proposals/slep002/proposal.rst:290: WARNING: 'any' reference target not found: Pipeline /home/nico/dev/enhancement_proposals/slep002/proposal.rst:293: WARNING: 'any' reference target not found: .fit /home/nico/dev/enhancement_proposals/slep002/proposal.rst:336: WARNING: 'any' reference target not found: pipe.add_* /home/nico/dev/enhancement_proposals/slep002/proposal.rst:424: WARNING: 'any' reference target not found: label_predictor /home/nico/dev/enhancement_proposals/slep002/proposal.rst:424: WARNING: 'any' reference target not found: ignore_predict /home/nico/dev/enhancement_proposals/slep002/proposal.rst:467: WARNING: 'any' reference target not found: __contains__() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:467: WARNING: 'any' reference target not found: __getitem__() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:467: WARNING: 'any' reference target not found: __setitem__() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:467: WARNING: 'any' reference target not found: __delitem__() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:468: WARNING: 'any' reference target not found: __len__() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:468: WARNING: 'any' reference target not found: __iter__() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:469: WARNING: 'any' reference target not found: __add__() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:469: WARNING: 'any' reference target not found: __iadd__() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:473: WARNING: 'any' reference target not found: get() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:473: WARNING: 'any' reference target not found: index() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:474: WARNING: 'any' reference target not found: extend() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:474: WARNING: 'any' reference target not found: insert() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:475: WARNING: 'any' reference target not found: keys() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:475: WARNING: 'any' reference target not found: items() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:475: WARNING: 'any' reference target not found: values() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:476: WARNING: 'any' reference target not found: clear() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:476: WARNING: 'any' reference target not found: pop() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:476: WARNING: 'any' reference target not found: popitem() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:476: WARNING: 'any' reference target not found: popfront() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:476: WARNING: 'any' reference target not found: popitemfront() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:481: WARNING: 'any' reference target not found: replace() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:482: WARNING: 'any' reference target not found: rename() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:490: WARNING: 'any' reference target not found: fromkeys() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:491: WARNING: 'any' reference target not found: setdefault() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:492: WARNING: 'any' reference target not found: sort() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:493: WARNING: 'any' reference target not found: __mul__() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:493: WARNING: 'any' reference target not found: __rmul__() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:493: WARNING: 'any' reference target not found: __imul__() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:495: WARNING: 'any' reference target not found: AttributeError /home/nico/dev/enhancement_proposals/slep002/proposal.rst:495: WARNING: 'any' reference target not found: NotImplementedError /home/nico/dev/enhancement_proposals/slep002/proposal.rst:500: WARNING: 'any' reference target not found: __ge__() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:500: WARNING: 'any' reference target not found: __gt__() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:501: WARNING: 'any' reference target not found: __le__() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:501: WARNING: 'any' reference target not found: __lt__() /home/nico/dev/enhancement_proposals/slep002/proposal.rst:507: WARNING: 'any' reference target not found: __getstate__ /home/nico/dev/enhancement_proposals/slep002/proposal.rst:507: WARNING: 'any' reference target not found: picklier /home/nico/dev/enhancement_proposals/slep002/proposal.rst:509: WARNING: 'any' reference target not found: .steps /home/nico/dev/enhancement_proposals/slep002/proposal.rst:551: WARNING: 'any' reference target not found: __init__ ```
amueller commented 5 years ago

Can you check what we do in sklearn? I think we may have some plugin or configure a fall-back? This seems to be a feature: https://www.sphinx-doc.org/en/master/usage/restructuredtext/roles.html#role-any

possibly we should be using double backticks for code segments like X_new, y_new, sample_props = estimator.fit_modify(X, y) if we can't make it fall back to that.

Not sure why it throws a warning for cross_val_score though.

NicolasHug commented 5 years ago

I don't think we're doing anything in scikit learn for this.

Compiling the docs yields tons of WARNING: 'any' reference target not found:.

As far as I understand any really only cares about references. It's not about using double backticks vs single backticks. Single backticks are for refs (and using 'any' just makes it easier, without having to specify e.g. :class: or :func:), and double backticks are for code formatting.

EDIT:

Hm that being said, there was no warning before I added 'any'...

amueller commented 5 years ago

My point was that if you use double backticks it'll just do code formatting and the any won't matter

NicolasHug commented 5 years ago

Yeah sure, I thought 'any' was introduced in sklearn so that we could use both simple and double backticks to format code (which I guess kinda works, expect that we get warnings?). I'm not sure.

Anyway, I used double backticks were appropriate, the only warning left is

enhancement_proposals/README.rst: WARNING: document isn't included in any toctree

Also, while intersphinx seems to be working properly, it doesn't look like we can write simply e.g. `cross_val_score`. We need the full `sklearn. ... .cross_val_score`. I think the only difference with/without 'any' is that we don't have to specify :func:

amueller commented 5 years ago

ok cool. I mean we don't need to polish everything in this PR imho and I'd be happy to merge.

amueller commented 5 years ago

The idea is that valid SLEPs will get merged initially without review, and hence will be visible online, then will be revised in pull requests?

exactly, that's how numpy does it. The current PRs are getting too long and convoluted I think, and provide too little visibility.

amueller commented 5 years ago

Anyone else want to weight in? @ogrisel @GaelVaroquaux @agramfort ?

Maybe we want "accepted", "rejected", "in process" and "backlog" headers, with all the current ones in "backlog", simliar to:

https://www.numpy.org/neps/

The reason why I want a "backlog" is that I would like to enforce some timing constraints on the discussion but I don't want all of the backlog to come and haunt us once we enact the governance doc.

agramfort commented 5 years ago

I would prefer "under review" than "in process"

I am fine with backlog but maybe "late review" or "delayed review" ?

amueller commented 5 years ago

thanks. merging this, given the lack of feedback. we can always go back /fix stuff.

amueller commented 5 years ago

@NicolasHug can you spin up readthedocs (do you need to be an org member for that?) and add a link into the readme here?