[WIP] Add verbose option for Pipeline and Feature Union [rebased 8568] by petrushev · Pull Request #9668 · scikit-learn/scikit-learn

petrushev · 2017-09-01T11:30:00Z

Reference Issue

What does this implement/fix? Explain your changes.

Rebases #8568 which has the following description:

This PR adds a new argument verbose to Pipeline and FeatureUnion, which is False by default. Setting this True will print useful information during execution.

Also, it extends the make_pipeline helper to support the verbose kwarg.

The snippets in the original PR produce the expected output.

Also checked with the example feature_selection/plot_feature_selection_pipeline.py which produces a nice output:

[Pipeline] (step 1 of 2) selectkbest ........................ 0.00057s
[Pipeline] (step 2 of 2) svc ................................ 0.00067s
[Pipeline] Total time elapsed: .............................. 0.00123s

- Each line printed by Pipeline and FeatureUnion, when their verbosity mode is on, will be 70 characters long.

petrushev · 2017-09-01T11:32:52Z

The whats new was not rebased correctly, will fix it.

glemaitre · 2017-09-01T11:34:12Z

sklearn/pipeline.py

+    print('%s%s%s' % (name, '.' * (70 - len(name + elapsed)), elapsed))
+
+
+class _BasePipeline(six.with_metaclass(ABCMeta, _BaseComposition)):


It seems that it has been badly merged. This class has been removed in favor of _BaseComposition.

glemaitre · 2017-09-01T11:35:04Z

sklearn/pipeline.py

 # License: BSD

 from collections import defaultdict
+from abc import ABCMeta, abstractmethod


not needed any more with _BaseComposition

glemaitre · 2017-09-01T11:36:35Z

sklearn/pipeline.py



-class FeatureUnion(_BaseComposition, TransformerMixin):
+class FeatureUnion(_BasePipeline, TransformerMixin):


It should also be subclassed from BaseComposition

glemaitre · 2017-09-01T11:38:07Z

sklearn/pipeline.py

+
+    def __init__(self, transformer_list, n_jobs=1, transformer_weights=None,
+                 verbose=False):
+        self.transformer_list = tosequence(transformer_list)


it should be list as previously stated

glemaitre · 2017-09-01T11:38:28Z

sklearn/pipeline.py

    def _update_transformer_list(self, transformers):
        transformers = iter(transformers)
-        self.transformer_list[:] = [
+        self.transformer_list = tosequence([


check master

Fix error message bug

glemaitre · 2017-09-02T13:58:21Z

@petrushev Could you solve the PEP8 issue:
https://travis-ci.org/scikit-learn/scikit-learn/jobs/270816940

glemaitre

@jnothman this PR has been resurrect and it seems to have been extensively reviewed previously.

Having a quick look, I was thinking that a decorator could be used in multiple places instead of repeating the same line around the processing. WDYT?

glemaitre · 2017-09-02T13:58:55Z

doc/whats_new.rst


 .. _Neeraj Gangwar: http://neerajgangwar.in
 .. _Arthur Mensch: https://amensch.fr
+


Remove this blank line.

glemaitre · 2017-09-02T13:59:38Z

doc/whats_new.rst

+- Added optional parameter ``verbose`` in functions `pipeline.make_pipeline`
+  and `pipeline.make_union` to extend the same functionality as the
+  corresponding classes. :issue:`9668` by
+  :user:`Baze Petrushev <petrushev>`.


if you could also add @karandesai-96 in the authorship since that it did a good part of the job as well :)

Thanks @glemaitre and @petrushev
Current What's New file on this branch has these two lines:

Miscellaneous - Added optional parameter ``verbose`` in :class:`pipeline.Pipeline` and :class:`pipeline.FeatureUnion` for showing progress and timing of each step. :issue:`8568` by :user:`Karan Desai <karandesai-96>`. - Added optional parameter ``verbose`` in functions `pipeline.make_pipeline` and `pipeline.make_union` to extend the same functionality as the corresponding classes. :issue:`9668` by :user:`Baze Petrushev <petrushev>`.

I think they should be unified outlining the feature, issue, PR and contributors.

glemaitre · 2017-09-02T14:00:37Z

sklearn/pipeline.py

 from .externals.joblib import Parallel, delayed
 from .externals import six
-from .utils.metaestimators import if_delegate_has_method
+from .utils.metaestimators import if_delegate_has_method, _BaseComposition


revert to the version of master with line 20 and line 24. It is equivalent to master in bring 2 diffs.

glemaitre · 2017-09-02T14:08:17Z

sklearn/pipeline.py


-def _fit_one_transformer(transformer, X, y):
-    return transformer.fit(X, y)
+def _fit_one_transformer(trans, X, y, verbose=False, idx=None,


replace trans by transformer in all this function

glemaitre · 2017-09-02T14:09:09Z

sklearn/pipeline.py

+                         total_steps=None, name=None):
+    # idx, total_steps and name are not required when verbosity is disabled
+    step_start_time = time.time()
+    trans = trans.fit(X, y)


trans -> transformer

glemaitre · 2017-09-02T14:10:35Z

sklearn/pipeline.py

-        return res, transformer
-    return res * weight, transformer
+        return res, trans
+    return res * weight, trans


trans -> transformer

glemaitre · 2017-09-02T14:12:53Z

sklearn/pipeline.py

            This estimator
        """
        self._validate_transformers()
+        all_transformers = [(name, trans, weight) for name, trans, weight in


This diff seems weird to me. It is not in master. I would be checking that this is not from the previous version before _BaseComposition

It seems this was done, along with the change in _fit_one_transform, for the benefit of the verbosity, so not in master.
Here 's the commit in the original PR.

glemaitre · 2017-09-02T14:13:32Z

sklearn/pipeline.py

            sum of n_components (output dimension) over transformers.
        """
        self._validate_transformers()
+        all_transformers = [(name, trans, weight) for name, trans, weight in


The same as the previous statement

glemaitre · 2017-09-02T14:15:09Z

sklearn/tests/test_pipeline.py


 class NoInvTransf(NoTrans):
-    def transform(self, X):
+    def transform(self, X, y=None):


Remove y=None. This is from the previous transformer version

glemaitre · 2017-09-02T14:15:20Z

sklearn/tests/test_pipeline.py


 class Transf(NoInvTransf):
-    def transform(self, X):
+    def transform(self, X, y=None):


Remove y=None

kdexd · 2017-09-03T03:44:32Z

Hi @petrushev, thanks for taking forward my PR.
@jnothman: I was amidst a lot of segfaults and failing tests of my codebase since past few weeks due to GSoC. I planned to work the remaining part of my PR after final evaluation, but it was hard to pull myself out. Nevertheless, I'm glad that this unmerged feature is approaching completion!

jnothman · 2017-09-04T00:47:11Z

Thanks for taking this on, @petrushev.

As stated in the previous PR, I had hoped we could share the logging logic with sklearn/model_selection/_validation.py's _fit_and_score by putting them in a sklearn/utils/ helper function. In particular, I thought we could share the formatting features, including the use of logger.short_format_time. Formatting times as %0.5f seems particularly weird, because I don't think 10 microseconds is a meaningful precision to report. It's this kind of discrepancy that makes me uncomfortable about reinventing the wheel, preferring to share the code.

petrushev · 2017-09-04T06:31:38Z

@jnothman Sure, that seems like the way to go. I'll see what I can do about it.

petrushev · 2017-09-05T22:32:07Z

@jnothman I wonder if this is something like you had in mind? The _fit_and_score is refactored in both _validation and cross_validation.

jnothman · 2017-09-05T23:27:20Z

it is, at a glance, thanks! I'll give it a more thorough review soon.

jnothman · 2017-09-05T23:32:36Z

could you please add a unit test for the helper, making sure it doesn't explode, for instance, if the text is too long. The standard library textwrap may prove useful for such things.

…

On 6 Sep 2017 9:27 am, "Joel Nothman" ***@***.***> wrote: it is, at a glance, thanks! I'll give it a more thorough review soon.

petrushev · 2017-09-06T08:51:28Z

of course, i just wanted to make sure i'm on the right track

amueller · 2017-10-24T21:09:28Z

are you still working on this?

petrushev · 2017-10-25T14:09:40Z

@amueller unfortunately i can't really promise i'll commit myself to it any time soon :/

jnothman · 2017-10-25T21:23:52Z

thanks for your work so far

amueller · 2017-10-25T21:35:45Z

@petrushev now worries and thanks for an honest assessment :)

Karan Desai added 4 commits September 1, 2017 12:27

ENH Add verbose option and corresponding tests for Pipeline.

ce53c81

ENH Add verbose option and corresponding tests for FeatureUnion.

c7bbb94

ENH Add _pretty_print method to print from Pipeline properly.

d1353fb

- Each line printed by Pipeline and FeatureUnion, when their verbosity mode is on, will be 70 characters long.

Add a changelog entry about verbosity of Pipeline.

a981bdc

glemaitre reviewed Sep 1, 2017

View reviewed changes

petrushev added 6 commits September 1, 2017 14:53

Fix bug in pipeline transformer list in-place assignment;

3bddca1

Fix error message bug

Extend make_pipeline to support the verbose kwarg as well

ba083b8

Fix whats new to reflect the Pipeline verbose change

5857d5e

Remove _BasePipeline resulting from a bad rebase

72f4d0e

Revert FeatureUnion.transformer_list to list instead of sequence

1b7245e

Extend FeatureUnion to support the verbose kwarg as well

3938adc

petrushev force-pushed the pipeline-verbose-rebased branch from 916663e to 3938adc Compare September 1, 2017 12:56

glemaitre reviewed Sep 2, 2017

View reviewed changes

Minor tidy-up

efb4aac

petrushev force-pushed the pipeline-verbose-rebased branch from 28740b6 to efb4aac Compare September 3, 2017 21:06

petrushev changed the title ~~Add verbose option for Pipeline and Feature Union [rebased 8568]~~ [WIP] Add verbose option for Pipeline and Feature Union [rebased 8568] Sep 3, 2017

Add message_with_time helper

2e5b6cc

petrushev force-pushed the pipeline-verbose-rebased branch from 39a0736 to 2e5b6cc Compare September 5, 2017 22:30

jnothman added Easy Well-defined and straightforward way to resolve help wanted Stalled labels Oct 25, 2017

jnothman mentioned this pull request Jan 9, 2018

[MRG] Add verbose option to Pipeline and FeatureUnion #10435

Closed

thomasjpfan mentioned this pull request Jun 27, 2018

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

Merged

jnothman closed this in #11364 Apr 21, 2019

		print('%s%s%s' % (name, '.' * (70 - len(name + elapsed)), elapsed))


		class _BasePipeline(six.with_metaclass(ABCMeta, _BaseComposition)):



		class FeatureUnion(_BaseComposition, TransformerMixin):
		class FeatureUnion(_BasePipeline, TransformerMixin):


		.. _Neeraj Gangwar: http://neerajgangwar.in
		.. _Arthur Mensch: https://amensch.fr

Uh oh!

Conversation

petrushev commented Sep 1, 2017

Reference Issue

What does this implement/fix? Explain your changes.

Uh oh!

petrushev commented Sep 1, 2017

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!

glemaitre commented Sep 2, 2017

Uh oh!

glemaitre left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

kdexd commented Sep 3, 2017

Uh oh!

jnothman commented Sep 4, 2017

Uh oh!

petrushev commented Sep 4, 2017

Uh oh!

petrushev commented Sep 5, 2017

Uh oh!

jnothman commented Sep 5, 2017 via email

Uh oh!

jnothman commented Sep 5, 2017 via email

Uh oh!

petrushev commented Sep 6, 2017

Uh oh!

amueller commented Oct 24, 2017

Uh oh!

petrushev commented Oct 25, 2017

Uh oh!

jnothman commented Oct 25, 2017 via email

Uh oh!

amueller commented Oct 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants