[MRG+1] - Voting classifier flatten transform (Continuation) #9188

herilalaina · 2017-06-20T22:03:55Z

Reference Issue

Fixes #7230, continuation of #7794

What does this implement/fix? Explain your changes.

Improve tests, fixes docstring and pep8 error

herilalaina · 2017-07-10T19:39:22Z

Ask for a review for this PR. @jnothman @amueller

jnothman

You've not tested the warning code, not silenced any warnings currently produced in testing or examples

jnothman · 2017-07-10T23:33:30Z

sklearn/ensemble/voting_classifier.py

@@ -163,6 +175,10 @@ def fit(self, X, y, sample_weight=None):
        if n_isnone == len(self.estimators):
            raise ValueError('All estimators are None. At least one is '
                             'required to be a classifier!')
+
+        if not self.flatten_transform and self.voting is 'soft':


This condition means the user who wants flatten_transform=False will always get the warning. We can avoid this by defaulting to some special marker, eg 'default'. We can tell the user in the warning message that setting it explicitly will Silence the warning

jnothman · 2017-07-10T23:36:31Z

Otherwise it's looking good

herilalaina · 2017-07-11T16:56:15Z

Thanks @jnothman Changes have been made.

jnothman · 2017-07-11T23:25:12Z

doc/whats_new.rst

+
+   - Added ``flatten_transform`` parameter to :class:`ensemble.VotingClassifier`
+     to change output shape of `transform` method to 2 dimensional.
+     :issue:`7794` by `Ibraim Ganiev <olologin>` and


Missing :user: here and in the next line

jnothman · 2017-07-11T23:27:27Z

sklearn/ensemble/voting_classifier.py

+                          " Setting it explicitly will silence this warning",
+                          DeprecationWarning)
+            warnings.warn("'flatten_transform' default value will be "
+                          "changed to True in 0.21.", DeprecationWarning)


I just meant to add here: To silence this warning you may explicitly set flatten_transform=False.

jnothman · 2017-07-11T23:29:07Z

sklearn/ensemble/voting_classifier.py

+                          DeprecationWarning)
+            warnings.warn("'flatten_transform' default value will be "
+                          "changed to True in 0.21.", DeprecationWarning)
+            self.flatten_transform = False


We shouldn't be modifying parameters. Just handle the default case in transform. In fact it makes sense to only raise this warning in transform as it won't be relevant to the majority of users who only care about prediction

herilalaina · 2017-07-12T00:29:16Z

I moved the warning into transform as you said. default is handle into False now. Since we want to change default into True in 0.21, should I add any deprecation warning in transform (like previous commit) ?

jnothman

Yes, of course the full deprecation warning belongs in transform

jnothman · 2017-07-12T01:47:11Z

The whole idea here is to maintain backwards compatibility, but to warn the users that they need to change their code to either maintain the old behaviour or adopt the new behaviour. Imagine using VotingClassifier(...='soft').fit_transform(X, y) now, then running the same code on a release with this PR merged.

jnothman · 2017-07-12T08:09:09Z

sklearn/ensemble/voting_classifier.py

+            if isinstance(self.flatten_transform,
+                          str) and self.flatten_transform == 'default':
+                warnings.warn("To silence this warning you may"
+                              " explicitly set flatten_transform=False",


Please merge these warnings into one

jnothman · 2017-07-12T08:29:35Z

sklearn/ensemble/voting_classifier.py

+            probas = self._collect_probas(X)
+            if isinstance(self.flatten_transform,
+                          str) and self.flatten_transform == 'default':
+                warnings.warn("To silence this warning you may"


Surely the first sentence only makes sense after the second.

jnothman

You've got it. LGTM!

herilalaina · 2017-07-12T09:59:30Z

Thanks for all your review

amueller

Looks good apart from nitpicks.

amueller · 2017-07-18T15:42:48Z

doc/whats_new.rst

@@ -284,6 +289,17 @@ Model evaluation and meta-estimators
   - Added ``sample_weight`` parameter to :meth:`pipeline.Pipeline.score`.
     :issue:`7723` by :user:`Mikhail Korobov <kmike>`.

+   - ``check_estimator`` now attempts to ensure that methods transform, predict, etc.


This seems unrelated.

amueller · 2017-07-18T15:43:22Z

sklearn/ensemble/voting_classifier.py

@@ -61,6 +62,12 @@ class VotingClassifier(_BaseComposition, ClassifierMixin, TransformerMixin):
        The number of jobs to run in parallel for ``fit``.
        If -1, then the number of jobs is set to the number of cores.

+    flatten_transform : bool, optional (default='default')


shouldn't it be None by default by convention?

amueller · 2017-07-18T15:43:57Z

sklearn/ensemble/voting_classifier.py

+    flatten_transform : bool, optional (default='default')
+        Affects shape of transform output only when voting='soft'
+        If voting='soft' and flatten_transform=True, transform method returns
+        matrix with shape (n_samples, n_classifiers * n_classes) instead of


maybe instead of "instead" say "if flatten_transform=False it returns"...

Maybe say what the current default behavior is and that it will change in the future. You can also use a versionadded sphinx directive here.

amueller · 2017-07-18T15:44:37Z

sklearn/ensemble/voting_classifier.py

@@ -256,16 +269,30 @@ def transform(self, X):

        Returns
        -------
-        If `voting='soft'`:
-          array-like = [n_classifiers, n_samples, n_classes]
+        If `voting='soft'` and `flatten_transform=False`:


And what if flatten_transform is True?

amueller · 2017-07-18T15:48:11Z

sklearn/ensemble/tests/test_voting_classifier.py

+        voting='soft',
+        flatten_transform=False).fit(X, y)
+
+    assert_array_equal(eclf1.transform(X).shape, (3, 4, 2))


can you wrap this in an assert_warns_message? Right now the warning is raised, and assert_warns_message actually returns the value, so you can then compare the shapes.

amueller · 2017-07-21T19:50:09Z

Do we want to backport this? Otherwise we need to move the whatsnew entry and change the deprecation version.

jnothman · 2017-07-22T21:58:02Z

I'm okay with backporting, but I'm not about to do it

…

On 22 Jul 2017 5:50 am, "Andreas Mueller" ***@***.***> wrote: Do we want to backport this? Otherwise we need to move the whatsnew entry and change the deprecation version. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9188 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6yKujO14P7AJ5jgSEEsBN7Evr5XBks5sQQDzgaJpZM4OANwo> .

amueller · 2017-07-23T01:47:14Z

@jnothman ok. I'll do all backporting in one go just before we release, I think.

jnothman reviewed Jul 10, 2017

View reviewed changes

herilalaina force-pushed the voting_classifier_flatten_transform branch 2 times, most recently from bf29f5f to 0926b4c Compare July 11, 2017 15:36

jnothman reviewed Jul 11, 2017

View reviewed changes

jnothman reviewed Jul 12, 2017

View reviewed changes

jnothman approved these changes Jul 12, 2017

View reviewed changes

jnothman changed the title ~~[MRG] - Voting classifier flatten transform (Continuation)~~ [MRG+1] - Voting classifier flatten transform (Continuation) Jul 12, 2017

amueller reviewed Jul 18, 2017

View reviewed changes

olologin and others added 14 commits July 18, 2017 20:16

flatten_transform parameter added to VotingClassifier

c52f314

Regression test added

e8f5e27

What's new section added

e8fd5e9

flake8 fix

d406b7b

Improve test and docstring

e3e5658

Add what's new entry

e0c70c5

default value flatten_transofrm

c7630cc

Add test for warning msg

8a5eb92

Fix bug in assert_warns_message

c7949d0

Move warn msg into transform

9dcaca8

Add deprecation warning

ffcc2ab

Merge warning

93dd0bd

Change warn msg

0db194b

Move what's content into Trees and ensembles

f1de47f

herilalaina force-pushed the voting_classifier_flatten_transform branch from 2e388bc to f1de47f Compare July 18, 2017 19:08

herilalaina added 3 commits July 18, 2017 21:15

Fixes minor bug

8340e29

update what's new

436b128

update test

f790b60

amueller merged commit 6f70202 into scikit-learn:master Jul 21, 2017

qinhanmin2014 mentioned this pull request Oct 18, 2017

Several fixed issues/PRs that might be closed #9948

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG+1] - Voting classifier flatten transform (Continuation) #9188

[MRG+1] - Voting classifier flatten transform (Continuation) #9188

herilalaina commented Jun 20, 2017

herilalaina commented Jul 10, 2017

jnothman left a comment

jnothman Jul 10, 2017

jnothman commented Jul 10, 2017

herilalaina commented Jul 11, 2017

jnothman Jul 11, 2017

jnothman Jul 11, 2017

jnothman Jul 11, 2017

herilalaina commented Jul 12, 2017

jnothman left a comment

jnothman commented Jul 12, 2017

jnothman Jul 12, 2017

jnothman Jul 12, 2017

jnothman left a comment

herilalaina commented Jul 12, 2017

amueller left a comment

amueller Jul 18, 2017

amueller Jul 18, 2017

amueller Jul 18, 2017

amueller Jul 18, 2017

amueller Jul 18, 2017

amueller Jul 18, 2017

amueller commented Jul 21, 2017

jnothman commented Jul 22, 2017 via email

amueller commented Jul 23, 2017

[MRG+1] - Voting classifier flatten transform (Continuation) #9188

[MRG+1] - Voting classifier flatten transform (Continuation) #9188

Conversation

herilalaina commented Jun 20, 2017

Reference Issue

What does this implement/fix? Explain your changes.

herilalaina commented Jul 10, 2017

jnothman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnothman commented Jul 10, 2017

herilalaina commented Jul 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

herilalaina commented Jul 12, 2017

jnothman left a comment

Choose a reason for hiding this comment

jnothman commented Jul 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnothman left a comment

Choose a reason for hiding this comment

herilalaina commented Jul 12, 2017

amueller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amueller commented Jul 21, 2017

jnothman commented Jul 22, 2017 via email

amueller commented Jul 23, 2017