DOC add a more complex example to gridsearch for nested parameters #14548

amueller · 2019-08-01T19:40:58Z

Related to what was brought up in #8710:
I don't think we explicitly describe how to do grid-searches on meta-estimators, and Pipeline actually works different from other meta-estimators (it uses the step names, not the parameter names).

codecov · 2019-08-02T18:38:38Z

Codecov Report

Merging #14548 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #14548      +/-   ##
==========================================
- Coverage   96.66%   96.65%   -0.02%     
==========================================
  Files         397      397              
  Lines       73015    73015              
  Branches     7980     7980              
==========================================
- Hits        70583    70575       -8     
- Misses       2419     2427       +8     
  Partials       13       13

Impacted Files	Coverage Δ
sklearn/_build_utils/__init__.py	`46.15% <0%> (-5.13%)`	⬇️
sklearn/svm/classes.py	`97.93% <0%> (-1.04%)`	⬇️
sklearn/metrics/pairwise.py	`97.25% <0%> (-0.83%)`	⬇️
sklearn/metrics/cluster/supervised.py	`97.95% <0%> (-0.69%)`	⬇️
sklearn/tree/export.py	`95.09% <0%> (-0.28%)`	⬇️
sklearn/cluster/tests/test_k_means.py	`99.79% <0%> (-0.21%)`	⬇️
sklearn/manifold/mds.py	`94.11% <0%> (+0.98%)`	⬆️

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 53f76d1...3f4558e. Read the comment docs.

doc/modules/compose.rst

thomasjpfan · 2019-08-02T20:18:30Z

doc/modules/grid_search.rst

+  >>> search = GridSearchCV(pipe, param_grid, cv=5)
+  >>> search.fit(X, y)
+  GridSearchCV(cv=5,
+               estimator=Pipeline(steps=[('select', SelectKBest()),


This is quite wide. Does doing something like this hide too much?

GridSearchCV(cv=5, estimator=Pipeline(steps=[...]), param_grid={'model__base_estimator__max_depth': [2, 4, 6, 8], 'select__k': [1, 2]})

it would be nice to format this differently but pytests doesn't allow me to.
We could change our repr lol.
I can also hide more with ellipsis but I think that's not super nice.

IGNORE_WHITESPACE doesn't allow changes in line breaks?? Who knew?

More practically, maybe just fit on the previous line and don't show the repr?

it allows replacing any whitespace by any other whitespace. There's no whitespace here to replace :-/

Co-Authored-By: Thomas J Fan <thomasjpfan@gmail.com>

doc/modules/grid_search.rst

jnothman · 2019-08-03T22:23:50Z

doc/modules/grid_search.rst

+
+Here, ``<estimator>`` is the parameter name of the nested estimator,
+in this case ``base_estimator``.
+The `Pipeline` class has a slightly different notation, as explained in :ref:`pipeline`,


It's not Pipeline alone... ColumnTransformer, VotingClassifier, etc, do this too... It's more like "If, like a Pipeline, the meta-estimator is constructed as a collection of other estimators, ..."

The other thing that's confusing about this is that, according to get_params, the step names are parameter names of the Pipeline. They're just not constructor params...

jnothman · 2019-08-03T22:24:43Z

doc/modules/grid_search.rst

+  >>> search = GridSearchCV(pipe, param_grid, cv=5)
+  >>> search.fit(X, y)
+  GridSearchCV(cv=5,
+               estimator=Pipeline(steps=[('select', SelectKBest()),


IGNORE_WHITESPACE doesn't allow changes in line breaks?? Who knew?

More practically, maybe just fit on the previous line and don't show the repr?

jnothman · 2019-08-03T22:26:39Z

Good that you've identified this... it's very hard to make sure there aren't gaps in the user guide when instead of fixing it, people go off writing books that cover the same material :D j/k

Co-Authored-By: Joel Nothman <joel.nothman@gmail.com>

amueller · 2019-08-05T15:20:06Z

doc/modules/grid_search.rst

+
+Here, ``<estimator>`` is the parameter name of the nested estimator,
+in this case ``base_estimator``.
+If the meta-estimator is constructed as a collection of estimators as in `Pipeline`, then


@jnothman better?

Pipeline should resolve to the Pipeline estimator doc, not to the user guide.

amueller · 2019-08-05T15:20:30Z

@jnothman if I had realized there was a gap here I would have fixed it before writing the book ;)

thomasjpfan · 2019-08-05T20:49:29Z

doc/modules/grid_search.rst

+  >>> search.fit(X, y)
+  GridSearchCV(cv=5,
+               estimator=CalibratedClassifierCV(base_estimator=RandomForestClassifier(n_estimators=10)),
+               param_grid={'base_estimator__max_depth': [2, 4, 6, 8]})


I was thinking of using ellipsis, which passes pytest.

GridSearchCV(cv=5, estimator=CalibratedClassifierCV(...), param_grid={'base_estimator__max_depth': [2, 4, 6, 8]})

Depends on if we prefer this:

or this:

adrinjalali · 2019-08-07T11:05:04Z

doc/modules/grid_search.rst

 -----------------------------------------
+`GridSearchCV` and `RandomizedSearchCV` allow searching over parameters of
+composite or nested estimators such as `Pipeline`, `ColumnTransformer`,
+`VotingClasssifier` or `CalibratedClassifierCV`


link resolution here is kinda funny:

why is VotingClassifier not resolved? Did I misspell it? Also: we can fix that later :P

Pipeline should resolve to the Pipeline estimator doc, not to the user guide.

Why? Isn't the user guide better?

IMO, not when all the others links are estimators and when the link is actually a title section that often doesn't make sense in the context they're used in.

maybe we should fix our title handles then? hm not sure, I don't have a strong opinion. I can use pipeline.Pipeline that should work, right? or use :ref:pipeline.Pipeline

Yeah, I also think they all should link to comparable pages, i.e. the API page in this case.

:class:`ensemble.VotingClassifier`

should work for VotingClassifier.

There was a typo ;)

NicolasHug

Bad magic link resolution :(

Other than that LGTM.

doc/modules/grid_search.rst

NicolasHug · 2019-08-07T19:06:16Z

doc/modules/grid_search.rst

 -----------------------------------------
+`GridSearchCV` and `RandomizedSearchCV` allow searching over parameters of
+composite or nested estimators such as `Pipeline`, `ColumnTransformer`,
+`VotingClasssifier` or `CalibratedClassifierCV`


Pipeline should resolve to the Pipeline estimator doc, not to the user guide.

NicolasHug · 2019-08-07T19:06:25Z

doc/modules/grid_search.rst

+
+Here, ``<estimator>`` is the parameter name of the nested estimator,
+in this case ``base_estimator``.
+If the meta-estimator is constructed as a collection of estimators as in `Pipeline`, then


Pipeline should resolve to the Pipeline estimator doc, not to the user guide.

Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>

adrinjalali · 2019-08-08T07:35:07Z

Pipeline still points to the user guide, doesn't it?

…it-learn into metaestimator_search_steps # Conflicts: # doc/modules/grid_search.rst

adrinjalali · 2019-08-09T08:01:28Z

doc/modules/grid_search.rst

 Composite estimators and parameter spaces
 -----------------------------------------
+`GridSearchCV` and `RandomizedSearchCV` allow searching over parameters of
+composite or nested estimators such as `Pipeline`, `ColumnTransformer`,


pipeline.Pipeline here too?

jnothman

Other lgtm

doc/modules/grid_search.rst

Co-Authored-By: Joel Nothman <joel.nothman@gmail.com>

add a more complex example to gridsearch for nested parameters

b8490a7

amueller mentioned this pull request Aug 1, 2019

CalibratedClassifierCV doesn't interact properly with Pipeline estimators #8710

Closed

amueller added 2 commits August 1, 2019 15:45

slight formatting fixes

c749e5d

normalize whitespace doesn't do what I thought it does

3f4558e

more whitespace yay

ddef0aa

thomasjpfan reviewed Aug 2, 2019

View reviewed changes

Update doc/modules/compose.rst

ee27265

Co-Authored-By: Thomas J Fan <thomasjpfan@gmail.com>

jnothman reviewed Aug 3, 2019

View reviewed changes

amueller and others added 3 commits August 5, 2019 11:13

Update doc/modules/grid_search.rst

630746d

Co-Authored-By: Joel Nothman <joel.nothman@gmail.com>

reformulate according to Joel's suggestions

a7da83e

typo

c6e7668

amueller commented Aug 5, 2019

View reviewed changes

thomasjpfan reviewed Aug 5, 2019

View reviewed changes

ellipsis

c226606

thomasjpfan approved these changes Aug 7, 2019

View reviewed changes

adrinjalali reviewed Aug 7, 2019

View reviewed changes

NicolasHug reviewed Aug 7, 2019

View reviewed changes

amueller and others added 2 commits August 7, 2019 15:32

Update doc/modules/grid_search.rst

f35cee2

Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>

Update doc/modules/grid_search.rst

190cbee

Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>

amueller added 2 commits August 8, 2019 12:25

change the link to point to api docs for pipeline

56ff5cc

Merge branch 'metaestimator_search_steps' of github.com:amueller/scik…

0445669

…it-learn into metaestimator_search_steps # Conflicts: # doc/modules/grid_search.rst

adrinjalali reviewed Aug 9, 2019

View reviewed changes

one more explicit use of pipeline module for linking to the API docs

21fa8b0

jnothman approved these changes Aug 12, 2019

View reviewed changes

doc/modules/grid_search.rst Outdated Show resolved Hide resolved

Update doc/modules/grid_search.rst

ea8efeb

Co-Authored-By: Joel Nothman <joel.nothman@gmail.com>

adrinjalali merged commit ec2ea1b into scikit-learn:master Aug 12, 2019

Uh oh!

DOC add a more complex example to gridsearch for nested parameters #14548

DOC add a more complex example to gridsearch for nested parameters #14548

Uh oh!

Conversation

amueller commented Aug 1, 2019

Uh oh!

codecov bot commented Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnothman commented Aug 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amueller commented Aug 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amueller Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasHug left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrinjalali commented Aug 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Aug 2, 2019 •

edited

Loading

jnothman commented Aug 3, 2019 •

edited

Loading

amueller Aug 7, 2019 •

edited

Loading