[MRG] Add verbose option for Pipeline and Feature Union. by kdexd · Pull Request #8568 · scikit-learn/scikit-learn

kdexd · 2017-03-10T03:09:35Z

Reference Issue

What does this implement/fix? Explain your changes.

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. Better to be explained by a code snippet:

Common code snippet (imports and data loading):

from sklearn.datasets import load_iris
from sklearn.decomposition import PCA
from sklearn.linear_model import LinearRegression
from sklearn.pipeline import Pipeline, FeatureUnion
from sklearn.preprocessing import Imputer

data = load_iris(return_X_y=True)

Pipeline Snippet:

p = Pipeline([('imputer', Imputer()), ('linreg', LinearRegression())],
             verbose=True)

p.fit(data[0], data[1])
y_pred = p.predict(data[0][0].reshape(1, -1))
print("Predicted: %f" % y_pred)

Expected Output:

[Pipeline] (step 1 of 2) imputer ............................ 0.00053s
[Pipeline] (step 2 of 2) linreg ............................. 0.00043s
[Pipeline] Total time elapsed: .............................. 0.00096s

FeatureUnion Snippet:

f = FeatureUnion([('imputer', StandardScaler()),
                  ('pca', PCA(n_components=2))], verbose=True)

f.fit(data[0], data[1])
data_t = f.transform(data[0][0].reshape(1, -1))
print("Transformed: %r" % data_t)

Expected Output:

[FeatureUnion] (step 1 of 2) imputer ........................ 0.00014s
[FeatureUnion] (step 2 of 2) pca ............................ 0.00032s
[FeatureUnion] Total time elapsed: .......................... 0.00065s
Transformed: array([[-0.90068117,  1.03205722, -1.3412724 , -1.31297673, -2.68420713,
         0.32660731]])

Any other comments?

TODO (will keep on editing this description):

Initial PR work, add verbosity to Pipeline.
Add tests for verbosity of Pipeline.
Add verbosity to FeatureUnion.
Add tests for verbosity to FeatureUnion.
Modify both methods to print in such a way that all lines have 70 characters.
Add CHANGELOG entry.

I had open PRs else where, so it took me a while to get back to this. Thanks @jnothman for mentioning me on issue thread.

kdexd · 2017-03-10T03:12:52Z

sklearn/pipeline.py

-            return last_step.fit(Xt, y, **fit_params).transform(Xt)
+            Xt = last_step.fit(Xt, y, **fit_params).transform(Xt)
+        if self.verbose:
+            self._print_final_step(final_step_start_time, time_elapsed_so_far)


I would have included if self.verbose condition in the method itself, to avoid repeated checks at every calls, but this way it felt semantically more appropriate - "if verbose then print final step".

Else simply the call would state "print final step" while it internally wouldn't have printed if not verbose. I hope that is fine ?

kdexd · 2017-03-10T11:54:17Z

Pipeline performs stepwise operations sequentially, while FeatureUnion executes parallel jobs for transformers. So for Pipeline, the total time elapsed will be sum of individual times taken at each step (and it is implemented that way too). On the other hand, total time elapsed for a FeatureUnion varies on the number of jobs, hence it is separately monitored, and so it necessarily need not be summation of individual times taken at each step..

raghavrv · 2017-03-10T17:09:54Z

This is a neat work... Thanks!

kdexd · 2017-03-10T17:15:38Z

Thanks @raghavrv 😄
Also, this PR if merged, should close #5321

raghavrv · 2017-03-13T07:18:41Z

doc/modules/pipeline.rst

    Pipeline(...,
             steps=[('reduce_dim', PCA(copy=True,...)),
-                    ('clf', SVC(C=1.0,...))])
+                    ('clf', SVC(C=1.0,...))], verbose=False)


Since this is an irrelevant detail, maybe use ellipsis? (...)

(I'm ambiguous on this...)

Well then i'll wait for others' suggestion meanwhile. I'm not sure which should be preferred either.

If we set an analogy, transformer_weights are shown here in same documentation. They're not having any value in this particular example.

>>> estimators = [('linear_pca', PCA()), ('kernel_pca', KernelPCA())] >>> combined = FeatureUnion(estimators) >>> combined # doctest: +NORMALIZE_WHITESPACE, +ELLIPSIS FeatureUnion(n_jobs=1, transformer_list=[('linear_pca', PCA(copy=True,...)), ('kernel_pca', KernelPCA(alpha=1.0,...))], transformer_weights=None, verbose=False)

We can use this as a tie breaker 😆

raghavrv · 2017-03-13T07:20:17Z

doc/whats_new.rst


+   - 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>`.


Please add your name and link to the bottom of the page :)

kdexd · 2017-04-10T04:28:06Z

/ping: @jnothman Resolved conflicts with master which moved forward after I opened my PR.

glemaitre · 2017-04-10T05:02:34Z

@karandesai-96 You should not merge master into your branch but rebase your branch to master.
Revert to the previous commit and rebase.

kdexd · 2017-04-10T05:06:05Z

@glemaitre I used the web interface for a very small conflict resolving, tried it for the first time. Agreed with you, not impressive.

Reverting, rebasing.

kdexd · 2017-04-10T05:09:59Z

Rebased.

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

jnothman · 2017-04-12T11:03:17Z

i often merge master into my branch. usually works just fine. I'll try look at this soon

…

On 10 Apr 2017 3:10 pm, "Karan Desai" ***@***.***> wrote: Rebased. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8568 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz67jDPLMIYHq-FvNnJaRYmbQzNPP6ks5rubmogaJpZM4MY6aU> .

glemaitre · 2017-04-12T11:29:55Z

I cannot say that this is a good practice or the right way to do it. I just got it while contributing in scikit-image in which I did merge and have been forced to reset and rebase: http://scikit-image.org/docs/0.11.x/contribute.html#divergence-between-upstream-master-and-your-feature-branch It seems that you have the same in the guide in scikit-learn: http://scikit-learn.org/stable/developers/contributing.html#how-to-contribute From the top of the head, I was thinking that the reasons were: merging make the history difficult to read but rebasing rewrite the history. -- Guillaume Lemaitre INRIA Saclay - Parietal team Center for Data Science Paris-Saclay https://glemaitre.github.io/

jnothman · 2017-04-12T11:59:37Z

I think Github deals with merging in master better than it used to. I no longer recommend rebases: that requires resolving conflicts on every commit rather than just between the heads (and squash-merging the final PR can handle whatever you did along the way as it'll just diff the final heads as long as they are merge-safe). It also messes up historical commit references and requires force pushes and other Dangerous Ideas.

…

On 12 April 2017 at 21:29, Guillaume Lemaitre ***@***.***> wrote: I cannot say that this is a good practice or the right way to do it. I just got it while contributing in scikit-image in which I did merge and have been forced to reset and rebase: http://scikit-image.org/docs/0.11.x/contribute.html# divergence-between-upstream-master-and-your-feature-branch It seems that you have the same in the guide in scikit-learn: http://scikit-learn.org/stable/developers/contributing.html#how-to- contribute From the top of the head, I was thinking that the reasons were: merging make the history difficult to read but rebasing rewrite the history. -- Guillaume Lemaitre INRIA Saclay - Parietal team Center for Data Science Paris-Saclay https://glemaitre.github.io/ — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8568 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6wjMpKHgN5NOr0o9iujLHWDvRSa6ks5rvLW0gaJpZM4MY6aU> .

glemaitre · 2017-04-12T12:31:16Z

I completely agree with the issue linked to resolving every conflicts, especially when resurrecting old PRs. In fact, I personally do not have a strong opinion regarding the policy to follow. Merge has only 1 con regarding the readability which is partially solved when we are actually squashing/merge at the end. Do you think that we should change the contributing guide in this regard if there is consensus behind a only merging policy?

…

-- Guillaume Lemaitre INRIA Saclay - Parietal team Center for Data Science Paris-Saclay https://glemaitre.github.io/

jnothman · 2017-04-12T22:21:43Z

I'd be in favour of updating the docs, yes

…

On 12 Apr 2017 10:31 pm, "Guillaume Lemaitre" ***@***.***> wrote: I completely agree with the issue linked to resolving every conflicts, especially when resurrecting old PRs. In fact, I personally do not have a strong opinion regarding the policy to follow. Merge has only 1 con regarding the readability which is partially solved when we are actually squashing/merge at the end. Do you think that we should change the contributing guide in this regard if there is consensus behind a only merging policy? -- Guillaume Lemaitre INRIA Saclay - Parietal team Center for Data Science Paris-Saclay https://glemaitre.github.io/ — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8568 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6wiC4PMDDhZmw4c8nRaBj9sEilLdks5rvMQWgaJpZM4MY6aU> .

jnothman · 2017-04-13T10:51:17Z

sklearn/pipeline.py

+
+    Parameters
+    ----------
+    step_info : str


Why is this preferred over description, elapsed or something else that further ensures consistency?

I would like to see logger.short_format_time(elapsed) used as in grid search, and this code refactored into utils with that.

What's the code in grid search you're talking about? The only logging is within joblib, right?

amueller · 2017-07-07T14:52:53Z

could you please resolve the conflicts?

amueller

I think this would be an awesome feature to have, but it should really live in a logger or something. Right now it's cluttering up the already complicated pipeline code.

kdexd · 2017-07-22T04:00:21Z

Hey @amueller, sorry I was away for a while I couldn't respond. I agree with your review. I will resolve the conflicts meanwhile I'll look into a more sane way for logging as you say.

jnothman · 2017-07-23T00:09:27Z

The verbosity from grid search lives in model_selection/_validate.py I think.

…

On 22 Jul 2017 2:00 pm, "Karan Desai" ***@***.***> wrote: Hey @amueller <https://github.com/amueller>, sorry I was away for a while I couldn't respond. I agree with your review. I will resolve the conflicts meanwhile I'll look into a more sane way for logging as you say. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8568 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz61rnEjFevRB91GKuw2gr5qQcQuAYks5sQXPXgaJpZM4MY6aU> .

amueller · 2017-07-23T01:38:18Z

Indeed, and it uses joblib.logger
https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/model_selection/_validation.py#L479

jnothman · 2017-08-23T08:09:46Z

I'd like to see this progress... Are you continuing to work on it @karandesai-96

EBazarov · 2017-10-24T17:05:33Z

Verbosity in pipeline very helpful feature, especially for debugging. Any plans to merge this PR ?

amueller · 2017-10-24T21:08:33Z

@EBazarov as soon as there is a reasonable implementation.

kdexd commented Mar 10, 2017

View reviewed changes

kdexd changed the title ~~[WIP] Add verbose option for Pipeline and Feature Union.~~ [MRG] Add verbose option for Pipeline and Feature Union. Mar 10, 2017

kdexd mentioned this pull request Mar 10, 2017

Verbose option on Pipeline #5298

Closed

raghavrv added Enhancement Waiting for Reviewer labels Mar 10, 2017

raghavrv reviewed Mar 13, 2017

View reviewed changes

Karan Desai added 2 commits April 10, 2017 10:39

ENH Add verbose option and corresponding tests for Pipeline.

09f3332

ENH Add verbose option and corresponding tests for FeatureUnion.

a990034

Karan Desai added 2 commits April 10, 2017 10:40

ENH Add _pretty_print method to print from Pipeline properly.

49eadee

- 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.

a3abe8d

jnothman reviewed Apr 13, 2017

View reviewed changes

glemaitre mentioned this pull request Apr 17, 2017

[MRG] DOC change rebase to merge policy #8753

Merged

amueller reviewed Jul 21, 2017

View reviewed changes

amueller removed the Waiting for Reviewer label Jul 21, 2017

petrushev mentioned this pull request Sep 1, 2017

[WIP] Add verbose option for Pipeline and Feature Union [rebased 8568] #9668

Closed

jnothman mentioned this pull request Jan 9, 2018

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

Closed

kdexd closed this Apr 1, 2018

kdexd deleted the pipeline-verbose branch April 1, 2018 04:37

thomasjpfan mentioned this pull request Jun 27, 2018

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

Merged

Uh oh!

Conversation

kdexd commented Mar 10, 2017 • edited by raghavrv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kdexd commented Mar 10, 2017

Uh oh!

raghavrv commented Mar 10, 2017

Uh oh!

kdexd commented Mar 10, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

kdexd Mar 13, 2017 • 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!

kdexd commented Apr 10, 2017

Uh oh!

glemaitre commented Apr 10, 2017

Uh oh!

kdexd commented Apr 10, 2017

Uh oh!

kdexd commented Apr 10, 2017

Uh oh!

jnothman commented Apr 12, 2017 via email

Uh oh!

glemaitre commented Apr 12, 2017 via email

Uh oh!

jnothman commented Apr 12, 2017 via email

Uh oh!

glemaitre commented Apr 12, 2017 via email

Uh oh!

jnothman commented Apr 12, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amueller commented Jul 7, 2017

Uh oh!

amueller left a comment

Choose a reason for hiding this comment

Uh oh!

kdexd commented Jul 22, 2017

Uh oh!

jnothman commented Jul 23, 2017 via email

Uh oh!

amueller commented Jul 23, 2017

Uh oh!

jnothman commented Aug 23, 2017

Uh oh!

EBazarov commented Oct 24, 2017

Uh oh!

amueller commented Oct 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kdexd commented Mar 10, 2017 •

edited by raghavrv

Loading

kdexd commented Mar 10, 2017 •

edited

Loading

kdexd Mar 13, 2017 •

edited

Loading