8000 [MRG]: Fixes Pipeline steps bug by thomasjpfan · Pull Request #12659 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[MRG]: Fixes Pipeline steps bug #12659

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Nov 26, 2018
Merged

[MRG]: Fixes Pipeline steps bug #12659

merged 4 commits into from
Nov 26, 2018

Conversation

thomasjpfan
Copy link
Member

What does this implement/fix? Explain your changes.

Fixes bug related to Pipeline's steps parameter. The idx used to modify the steps may not correspond to the correct step when None or passthrough is used.

On master, this test will fail:

@pytest.mark.parametrize('passthrough', [None, 'passthrough'])
def test_pipeline_correctly_adjusts_steps(passthrough):
    X = np.array([[1]])
    y = np.array([1])
    mult2 = Mult(mult=2)
    mult3 = Mult(mult=3)
    mult5 = Mult(mult=5)

    pipeline = Pipeline([
        ('m2', mult2),
        ('bad', passthrough),
        ('m3', mult3),
        ('m5', mult5)
    ])

    pipeline.fit(X, y)
    expected_names = ['m2', 'bad', 'm3', 'm5']
    actual_names = [name for name, _ in pipeline.steps]
    assert expected_names == actual_names

8000 Specifically, on master, pipeline.steps would be equal to ['m2', 'm3', 'm3', 'm5']. This PR adjusts Pipeline._iter to also yield the step's index.

@thomasjpfan thomasjpfan mentioned this pull request Nov 23, 2018
Closed
rth
rth approved these changes Nov 23, 2018
View reviewed changes
Copy link
Member
@rth rth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @thomasjpfan ! Please add a what's new entry.

@jorisvandenbossche
Copy link
Member

Please add a what's new entry.

This looks like a regression in case of None (and in any case a small bug fix), so I think this can certainly be added to the 0.20 whatsnew file to have this in 0.20.2

@jorisvandenbossche jorisvandenbossche mentioned this pull request Nov 23, 2018
Merged
@jnothman jnothman added this to the 0.20.2 milestone Nov 26, 2018
@jnothman jnothman merged commit 1b90237 into scikit-learn:master Nov 26, 2018
@amueller
Copy link
Member

@jorisvandenbossche but "passthrough" is not in 0.20.1, so I'm not sure how to backport this?
8000

@amueller amueller mentioned this pull request Dec 14, 2018
Merged
@thomasjpfan
Copy link
Member Author

The "passthrough" feature was intended for 0.21, so this PR would not be able to be backported: #11674

@thomasjpfan thomasjpfan mentioned this pull request Dec 14, 2018
Closed
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
@thomasjpfan
FIX Pipeline steps incorrectly updated with passthrough step (scikit-…
408e51b 
…learn#12659)
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
Revert "FIX Pipeline steps incorrectly updated with passthrough step (s…
a445082 
…cikit-learn#12659)"

This reverts commit 408e51b.
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
Revert "FIX Pipeline steps incorrectly updated with passthrough step (s…
d35f563 
…cikit-learn#12659)"

This reverts commit 408e51b.
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
@thomasjpfan @koenvandevelde
FIX Pipeline steps incorrectly updated with passthrough step (scikit-…
477d3e4 
…learn#12659)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnothman jnothman jnothman approved these changes

@rth rth rth approved these changes

@jorisvandenbossche jorisvandenbossche jorisvandenbossche approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.20.2
Development

Successfully merging this pull request may close these issues.
5 participants
@thomasjpfan @jorisvandenbossche @amueller @jnothman @rth
0