Skip to content
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.

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

[MRG+1] Add verbose option to Pipeline, FeatureUnion, and ColumnTransformer #11364

Merged
merged 80 commits into from
Apr 21, 2019

Conversation

thomasjpfan
Copy link
Member

Reference Issues/PRs

Continues work on PR #10435.

Fixes #9668. Fixes #8568. Fixes #5298. Fixes #5321.

What does this implement/fix? Explain your changes.

Adapts the new functions in pipeline.py to work with ColumnTransformer.

Karan Desai and others added 24 commits September 1, 2017 12:27
- Each line printed by Pipeline and FeatureUnion, when their
  verbosity mode is on, will be 70 characters long.
# Conflicts:
#	sklearn/pipeline.py
#	sklearn/utils/__init__.py
#	sklearn/utils/tests/test_utils.py
@jnothman
Copy link
Member

Thanks for fixing that up. This is nice.
Of course, it wouldn't hurt to provide verbose in ColumnTransformer if we do so in FeatureUnion.

@thomasjpfan thomasjpfan changed the title MRG: Add verbose option to Pipeline and FeatureUnion while fixing ColumnTransformer MRG: Add verbose option to Pipeline, FeatureUnion, and ColumnTransformer Jun 27, 2018
@thomasjpfan
Copy link
Member Author

I updated this PR with verbose support in ColumnTransformer.

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Some first comments

Additionally, make_column_transformer is missing support for verbose attribute

doc/whats_new/v0.20.rst Outdated Show resolved Hide resolved
sklearn/compose/_column_transformer.py Outdated Show resolved Hide resolved
sklearn/pipeline.py Outdated Show resolved Hide resolved
sklearn/pipeline.py Outdated Show resolved Hide resolved
sklearn/utils/__init__.py Outdated Show resolved Hide resolved
.format(list(kwargs.keys())[0]))
return FeatureUnion(_name_estimators(transformers), n_jobs=n_jobs)
raise TypeError('Unknown keyword arguments: "{}"'.format(
list(kwargs.keys())[0]))
Copy link
Member

Choose a reason for hiding this comment

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

hasn't been addressed

sklearn/tests/test_pipeline.py Outdated Show resolved Hide resolved
sklearn/pipeline.py Outdated Show resolved Hide resolved
sklearn/pipeline.py Outdated Show resolved Hide resolved
sklearn/pipeline.py Outdated Show resolved Hide resolved
sklearn/pipeline.py Outdated Show resolved Hide resolved
sklearn/pipeline.py Outdated Show resolved Hide resolved
sklearn/utils/__init__.py Outdated Show resolved Hide resolved
[est for _, est in self.steps[:step_idx + 1] if est is not None])
return '(step %d of %d) Fitting %s' % (step_num, n_steps,
self.steps[step_idx][0])

# Estimator interface

def _fit(self, X, y=None, **fit_params):
Copy link
Member

Choose a reason for hiding this comment

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

Not strictly related to this PR, but I find it confusing that this is called _fit while it actually does some transforming jobs as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mind as well call it _fit_transform or if we want to be more specific: _fit_transform_without_final :)

Copy link
Member

Choose a reason for hiding this comment

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

_fit_transform_without_final sounds great to me

sklearn/pipeline.py Outdated Show resolved Hide resolved
@thomasjpfan
Copy link
Member Author

This PR has been updated to use the phrase "Processing _____" when returning the verbose output.

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Nitpicks but LGTM

Xt = last_step.fit_transform(Xt, y, **fit_params)
else:
Xt = last_step.fit(Xt, y, **fit_params).transform(Xt)
return Xt
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should use

if... : 
    return Xt
elif...:
    return ...
else:
    return ...

"""
Fits ``transformer`` to ``X`` and ``y``. The transformed result is returned
with the fitted transformer. If ``weight`` is not ``None``, the result will
be multipled by ``weight``.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
be multipled by ``weight``.
be multiplied by ``weight``.

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Nitpicks but LGTM

@jnothman
Copy link
Member

Let's ship it! Thanks for wrapping it up, @thomasjpfan

@jnothman jnothman merged commit 8edd9f9 into scikit-learn:master Apr 21, 2019
Sprint Paris 2019 automation moved this from To do to Done Apr 21, 2019
jeremiedbb pushed a commit to jeremiedbb/scikit-learn that referenced this pull request Apr 25, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
@amueller amueller removed this from PR phase in Andy's pets Jul 29, 2019
hannahbrucemacdonald added a commit to hannahbrucemacdonald/scikit-learn that referenced this pull request Aug 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Verbose option on Pipeline
8 participants