[MRG+2] RFC: Adds passthrough option to Pipeline #11674

thomasjpfan · 2018-07-25T00:06:42Z

Reference Issues/PRs

Fixes #11144

What does this implement/fix? Explain your changes.

Adds 'passthrough' to pipeline.Pipeline. I added a Pipeline._iter method to handle the filtering of transformers.

jnothman

Is there not an example that uses this in the gallery? It would also need updating

thomasjpfan · 2018-07-25T16:45:52Z

I ran the following to see if there are any references to None together with Pipeline in the examples:

grep -rl "Pipeline" examples | xargs grep "None"

There does not seem to be an example that uses the None feature with Pipeline.

jnothman

I'm not entirely happy with the creation of _iter. The logic is now harder to read because of it, particularly because of the default include_final_estimator behaviour.

But I'm not entirely upset about it either.

Something you need to check here and in the FeatureUnion PR: what currently happens with make_{pipeline,union} including None? Does it work? What does it get named?

jnothman · 2018-07-25T07:46:57Z

sklearn/pipeline.py

+
+    def _iter(self, include_final_estimator=False):
+        """
+        Generate (name, trans) tuples excluding None and


Better to not mention None anymore, or it makes completing the deprecation harder.

thomasjpfan · 2018-07-26T10:57:09Z

_iter was created because of two reasons:

Iteration and filtering out 'passthrough' (or the deprecated None) is common in Pipeline
When None gets deprecated, removing the None check in _iter would be most of the work needed.

I made include_final_estimator=False the default, because it was the most common use case. I updated the PR to change the default to include_final_estimator=True.

When make_{pipeline,union}(None) gets call, it creates an estimator named 'nonetype' with None as its estimator. This naming is done in pipeline._name_estimators.

jnothman · 2018-07-27T02:29:45Z

Let's make the parameter with_final Supporting passthrough or drop in make_* is a bit awkward...

…sthrough

thomasjpfan · 2018-07-27T15:10:03Z

It is a bit strange. Although, I can see some workflows where users are creating pipelines dynamically, and end up with make_pipeline(None) or make_pipeline('passthrough').

jnothman · 2018-07-29T03:55:00Z

Okay, so let's either forbid it or make sure 'passthrough' gets named sensibly.

…sthrough

thomasjpfan · 2018-08-18T23:23:42Z

This PR was updated to just name the passthrough 'passthrough' when calling make_pipeline('passthrough'). The behavior for make_pipeline(None) is left unchanged.

jnothman · 2018-08-18T23:34:32Z

I'm now wondering if this change is truly worth the hassle to users.

thomasjpfan · 2018-08-19T00:07:25Z

I believe the point of this change was to unify the api of ColumnTransformer, Pipeline, and FeatureUnion. Currently, Pipeline uses None for passthrough , and FeatureUnion uses None for dropping.

This PR together with #11640 would unify the API.

jnothman · 2018-08-19T01:29:14Z

< 8000 /tr>

yes, I get that... but I'm not sure that deprecating None support is anything but a nuisance for users.

jorisvandenbossche

Looks good to me, added some minor comments.

Maybe also add a test with another string than 'passthrough' (to test that it properly errors)

jorisvandenbossche · 2018-08-23T15:10:15Z

sklearn/pipeline.py

-        transformers = estimators[:-1]
-        estimator = estimators[-1]
+        transformers_steps = self.steps[:-1]
+        estimator_name, estimator = self.steps[-1]


Why did you change this? As it doesn't look like the names are actually used?

When we were still deprecating None, this provided the name fo the transformer for the warning message. This is not needed anymore.

jorisvandenbossche · 2018-08-23T15:20:41Z

sklearn/pipeline.py

@@ -167,10 +168,23 @@ def _validate_steps(self):
                                " '%s' (type %s) doesn't" % (t, type(t)))


I would add here something like: or be the string "passthrough"

jorisvandenbossche · 2018-08-23T15:20:59Z

sklearn/pipeline.py

@@ -527,7 +533,11 @@ def _pairwise(self):
 def _name_estimators(estimators):
    """Generate names for estimators."""

-    names = [type(estimator).__name__.lower() for estimator in estimators]
+    names = [
+        estimator.lower()


is the lower needed? (do we support somebody passing 'Passthrough' ?)

jorisvandenbossche · 2018-08-23T15:23:21Z

sklearn/tests/test_pipeline.py

+    pipeline = make_pipeline(None)
+
+
+def test_set_pipeline_step_passthrough():


Didn't look in detail, but it might be relatively easy to parametrize this test to test both None and 'passthrough' ? (then you don't need two tests that duplicates things)

…sthrough

amueller · 2018-10-04T13:55:38Z

There's no tests that None still works, right? (there should be)

thomasjpfan · 2018-10-04T15:38:34Z

@amueller The tests are there defined at test_set_pipeline_step_passthrough:

@pytest.mark.parametrize('passthrough', [None, 'passthrough'])
def test_set_pipeline_step_passthrough(passthrough):
    ...

The parametrize allows it to test for both None and 'passthrough'.

amueller · 2018-10-04T15:41:17Z

Ah, you're right. I was reading to quickly. Looks good.

ngoix · 2018-10-09T06:09:21Z

examples/compose/plot_compare_reduction.py needs now to be updated (after #12272)

…sthrough

thomasjpfan · 2018-10-09T17:27:59Z

This PR keeps both options, None and 'passthrough', available. I still updated the examples/compose/plot_compare_reduction.py just to nudge new users to use the 'passthrough' option.

jnothman

I've not looked through this again in detail, but I think it's good.

jnothman · 2018-10-10T01:53:14Z

doc/whats_new/v0.20.rst

@@ -945,6 +945,9 @@ Support for Python 3.3 has been officially dropped.
 - |API| :class:`pipeline.FeatureUnion` now supports ``'drop'`` as a transformer
  to drop features. :issue:`11144` by :user:`thomasjpfan`.

+- |API| :class:`pipeline.Pipeline` now supports using ``'passthrough'`` as a 


Please move to 0.21.

jnothman · 2018-11-05T02:22:05Z

Thanks again, @thomasjpfan!

This reverts commit 5098474.

thomasjpfan added 2 commits July 24, 2018 20:03

RFC: Uses passthrough instead of None in Pipeline

30583a2

DOC: Fix

d3a9705

jnothman reviewed Jul 25, 2018

View reviewed changes

jnothman reviewed Jul 26, 2018

View reviewed changes

RFC: Change default to True

c83456b

thomasjpfan force-pushed the pipeline_drop_passthrough branch from cb3a1d2 to c83456b Compare July 26, 2018 15:26

thomasjpfan added 3 commits July 27, 2018 10:52

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

65f77ab

…sthrough

RFC: Changes keyword name

33ff1f6

DOC: Fix

7b5a96c

thomasjpfan changed the title ~~RFC: Uses passthrough instead of None in Pipeline~~ [MRG] RFC: Uses passthrough instead of None in Pipeline Jul 27, 2018

thomasjpfan added 2 commits July 29, 2018 00:13

ENH: Names passthrough im make_pipeline

61d9abb

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

800a87a

…sthrough

thomasjpfan added 4 commits August 22, 2018 14:25

REV: Does not deprecate None

49cd6de

DOC: Formatting

f8e2582

DOC: Update whats_new

e709f86

STY: flake8

72786b8

jorisvandenbossche reviewed Aug 23, 2018

View reviewed changes

thomasjpfan changed the title ~~[MRG] RFC: Uses passthrough instead of None in Pipeline~~ [MRG] RFC: Adds passthrough option to Pipeline Aug 23, 2018

thomasjpfan added 3 commits August 23, 2018 12:00

RFC: Removes name

a6c6699

ENH: Adds detail to warning

354a57a

RFC: Removes lower for estimator name

c89e195

thomasjpfan added 5 commits August 23, 2018 12:06

TST: New warning message

e4dbef6

TST: Combines None and passthrough tests

fc69bfb

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

568d40b

…sthrough

STY: flake8

9d72913

BLD: Trigger CI

6ed9a7c

BLD: Trigger CI

6ed9a7c

thomasjpfan added 2 commits October 9, 2018 12:11

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

410a32d

…sthrough

DOC: Uses passthrough instead of None

322baa5

amueller approved these changes Oct 10, 2018

View reviewed changes

jnothman approved these changes Oct 10, 2018

View reviewed changes

DOC: Move to v0.21

cf3ff10

thomasjpfan changed the title ~~[MRG] RFC: Adds passthrough option to Pipeline~~ [MRG+2] RFC: Adds passthrough option to Pipeline Nov 2, 2018

jnothman merged commit 9f842e3 into scikit-learn:master Nov 5, 2018

thoo pushed a commit to thoo/scikit-learn that referenced this pull request Nov 14, 2018

API Adds passthrough option to Pipeline (scikit-learn#11674)

485ed35

thomasjpfan mentioned this pull request Dec 14, 2018

[MRG]: Fixes Pipeline steps bug #12659

Merged

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

API Adds passthrough option to Pipeline (scikit-learn#11674)

5098474

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

Revert "API Adds passthrough option to Pipeline (scikit-learn#11674)"

10a7473

This reverts commit 5098474.

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

Revert "API Adds passthrough option to Pipeline (scikit-learn#11674)"

02b51d9

This reverts commit 5098474.

koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019

API Adds passthrough option to Pipeline (scikit-learn#11674)

e12fa0d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[MRG+2] RFC: Adds passthrough option to Pipeline #11674

[MRG+2] RFC: Adds passthrough option to Pipeline #11674

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		@@ -167,10 +168,23 @@ def _validate_steps(self):
		" '%s' (type %s) doesn't" % (t, type(t)))

		pipeline = make_pipeline(None)


		def test_set_pipeline_step_passthrough():

Uh oh!

[MRG+2] RFC: Adds passthrough option to Pipeline #11674

[MRG+2] RFC: Adds passthrough option to Pipeline #11674

Uh oh!

Conversation

Uh oh!

Reference Issues/PRs

What does this implement/fix? Explain your changes.

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!