[MRG+2] chore(make_union): add n_jobs to make_union through kwargs #8031

alexandercbooth · 2016-12-10T05:18:25Z

Addresses #8028 by adding kwargs to make_union.

…lank line

…se flake8 linter

jnothman

otherwise, this is looking good. thanks for the neat test.

jnothman · 2016-12-11T09:56:46Z

sklearn/pipeline.py

@@ -792,6 +790,7 @@ def make_union(*transformers):
    Examples


We should really have a Parameters section here in the docstring.

jnothman · 2016-12-11T09:58:02Z

sklearn/tests/test_pipeline.py

+    pca = PCA(svd_solver='full')
+    mock = Transf()
+    fu = make_union(pca, mock, n_jobs=3)
+    assert_equal(3, fu.n_jobs)


you might, for completeness, check that fu.transformers == make_union(pca, mock).transformers but this is fine as is.

jnothman · 2016-12-12T02:17:24Z

sklearn/pipeline.py

@@ -787,6 +787,23 @@ def make_union(*transformers, **kwargs):
    and does not permit, naming the transformers. Instead, they will be given
    names automatically based on their types. It also does not allow weighting.

+    Parameters
+    ----------
+    transformer_list : list of (string, transformer) tuples


This is not correct. You want something like. *transformers : list of estimators

jnothman · 2016-12-12T02:18:00Z

sklearn/pipeline.py

+    n_jobs : int, optional
+        Number of jobs to run in parallel (default 1).
+
+    transformer_weights : dict, optional


Should probably leave this out, given that transformer names are automatically assigned, so it only really adds confusion.

Good point, thanks!

amueller · 2016-12-12T17:02:21Z

sklearn/tests/test_pipeline.py

@@ -418,6 +418,14 @@ def test_make_union():
    assert_equal(transformers, (pca, mock))


+def test_make_union_kwargs():


maybe add a test that if an invalid **kwarg is given, there is an error? Otherwise LGTM.

Great, added an extra assertion that raises a TypeError. Thanks!

amueller · 2016-12-12T18:02:59Z

sklearn/tests/test_pipeline.py

+    fu = make_union(pca, mock, n_jobs=3)
+    assert_equal(fu.transformer_list, make_union(pca, mock).transformer_list)
+    assert_equal(3, fu.n_jobs)
+    assert_raises(TypeError, make_union,pca, mock, invalidFakeKwarg=42)


you can use assert_raise_message to check if the error message is helpful but it's also fine as-is.

Good thinking, the error message is helpful so I will go that route instead.

amueller · 2016-12-12T19:46:17Z

sklearn/tests/test_pipeline.py

+    # invalid keyword parameters should raise an error message
+    assert_raise_message(
+        TypeError,
+        "__init__() got an unexpected keyword argument 'invalidFakeKwarg'",


amueller · 2016-12-12T19:46:41Z

LGTM!

jnothman · 2016-12-14T10:31:21Z

sklearn/pipeline.py

    """
-    return FeatureUnion(_name_estimators(transformers))
+    return FeatureUnion(_name_estimators(transformers), **kwargs)


Perhaps we should check, until we decide, perhaps, to implement some kind of transformer_weights, that only n_jobs is passed in. Sometimes it's better to be conservative. WDYT?

So error here if something is given that would be accepted by FeatureUnion but could be garbled? I'm not entirely sure what the risks are, but I'm happy to play it safe.

I like the idea of being conservative, @jnothman

Maybe something like the following (but perhaps with a KeyError instead)?

def make_union(*transformers, **kwargs): """Construct a FeatureUnion from the given transformers. This is a shorthand for the FeatureUnion constructor; it does not require, and does not permit, naming the transformers. Instead, they will be given names automatically based on their types. It also does not allow weighting. Parameters ---------- *transformers : list of estimators n_jobs : int, optional Number of jobs to run in parallel (default 1). Returns ------- f : FeatureUnion Examples -------- >>> from sklearn.decomposition import PCA, TruncatedSVD >>> from sklearn.pipeline import make_union >>> make_union(PCA(), TruncatedSVD()) # doctest: +NORMALIZE_WHITESPACE FeatureUnion(n_jobs=1, transformer_list=[('pca', PCA(copy=True, iterated_power='auto', n_components=None, random_state=None, svd_solver='auto', tol=0.0, whiten=False)), ('truncatedsvd', TruncatedSVD(algorithm='randomized', n_components=2, n_iter=5, random_state=None, tol=0.0))], transformer_weights=None) """ if 'n_jobs' in kwargs: return FeatureUnion(_name_estimators(transformers), **kwargs) elif kwargs: raise NotImplementedError else: return FeatureUnion(_name_estimators(transformers))

This gives us the following output:

In [9]: make_union(PCA(), TruncatedSVD()) Out[9]: FeatureUnion(n_jobs=1, transformer_list=[('pca', PCA(copy=True, iterated_power='auto', n_components=None, random_state=None, svd_solver='auto', tol=0.0, whiten=False)), ('truncatedsvd', TruncatedSVD(algorithm='randomized', n_components=2, n_iter=5, random_state=None, tol=0.0))], transformer_weights=None) In [10]: make_union(PCA(), TruncatedSVD(), n_jobs=4) Out[10]: FeatureUnion(n_jobs=4, transformer_list=[('pca', PCA(copy=True, iterated_power='auto', n_components=None, random_state=None, svd_solver='auto', tol=0.0, whiten=False)), ('truncatedsvd', TruncatedSVD(algorithm='randomized', n_components=2, n_iter=5, random_state=None, tol=0.0))], transformer_weights=None) In [11]: make_union(PCA(), TruncatedSVD(), anyOtherArg=3) --------------------------------------------------------------------------- NotImplementedError Traceback (most recent call last) <ipython-input-11-87bdeace7aa2> in <module>() ----> 1 make_union(PCA(), TruncatedSVD(), anyOtherArg=3) /Users/acb/Documents/DataSci/contrib/scikit/scikit-learn/sklearn/pipeline.py in make_union(*transformers, **kwargs) 818 return FeatureUnion(_name_estimators(transformers), **kwargs) 819 elif kwargs: --> 820 raise NotImplementedError 821 else: 822 return FeatureUnion(_name_estimators(transformers)) NotImplementedError:

Thank you both for all your input on this 👍

I think you want something more like

n_jobs = kwargs.pop('n_jobs', None) if kwargs: # We do not currently support `transformer_weights` as we may want to change its type spec in make_union raise TypeError('Unknown keyword arguments: {}'.format(kwargs.keys())) FeatureUnion(_name_estimators(transformers), n_jobs=n_jobs)

OK, sounds good. Would we want the default n_jobs to be 1 instead of None to keep it consistent?

jnothman · 2016-12-15T01:23:02Z

yes, 1

…

On 15 December 2016 at 12:22, alexandercbooth ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In sklearn/pipeline.py <#8031>: > """ - return FeatureUnion(_name_estimators(transformers)) + return FeatureUnion(_name_estimators(transformers), **kwargs) OK, sounds good. Would we want the default n_jobs to be 1 instead of None to keep it consistent? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8031>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6xzyQrdggBDIEvEkZK-FLVVCowhkks5rIJY6gaJpZM4LJkCc> .

jnothman

We could also include transformer_weights in the test. Otherwise, this LGTM.

alexandercbooth · 2016-12-15T13:52:50Z

That's a good point as the most likely kwarg people might try there is transformer_weights. That will make more sense to test rather than the fake kwarg I had before. Thanks!

jnothman · 2016-12-15T13:55:39Z

Please add an Enhancements entry into whats_new.rst. Thanks!!

alexandercbooth · 2016-12-15T14:49:51Z

Thanks!

jnothman · 2016-12-15T21:05:03Z

In the future, please use "Fixes #xxxx" in the PR description so the issue closes automatically when PR is merged.

alexandercbooth · 2016-12-15T21:09:07Z

Will do, sincere thanks for all you help, @jnothman!

alexandercbooth added 4 commits December 10, 2016 00:15

[MRG] [doc skip] chore(make_union): add n_jobs through kwargs

30bcf0b

[doc skip] test(make_union): added make_union test that tests n_jobs

7367dea

[doc skip] chore(make_union): appease flake8 linter by adding extra b…

ec36f48

…lank line

[doc skip] test(make_union): remove whitespace from new test to appea…

adb2661

…se flake8 linter

jnothman reviewed Dec 11, 2016

View reviewed changes

alexandercbooth added 2 commits December 11, 2016 16:38

chore(make_union): added parameters to docstring

c5e941f

chore(make_union_kwargs): added transformer check to this test

0c24635

jnothman requested changes Dec 12, 2016

View reviewed changes

chore(make_union): format docstring

969c4c5

amueller reviewed Dec 12, 2016

View reviewed changes

test(make_union_kwarg): added assertion raise for TypeError

5bad203

amueller reviewed Dec 12, 2016

View reviewed changes

amueller changed the title ~~[MRG] chore(make_union): add n_jobs to make_union through kwargs~~ [MRG + 1] chore(make_union): add n_jobs to make_union through kwargs Dec 12, 2016

test(make_union_kwarg): change assertion error to check for message

5b610ae

amueller reviewed Dec 12, 2016

View reviewed changes

jnothman reviewed Dec 14, 2016

View reviewed changes

alexandercbooth added 2 commits December 14, 2016 17:40

chore(make_union): restrict kwargs to n_jobs only

c4a266e

test(make_union_kwarg): refactor test to conform to kwarg restriction

2e2bb13

jnothman reviewed Dec 15, 2016

View reviewed changes

chore(make_union_kwargs): use transformer_weights as the invalid kwarg

4d2aadc

jnothman approved these changes Dec 15, 2016

View reviewed changes

jnothman changed the title ~~[MRG + 1] chore(make_union): add n_jobs to make_union through kwargs~~ [MRG+2] chore(make_union): add n_jobs to make_union through kwargs Dec 15, 2016

docs(make_union): add make_union changes to whats new

f97abac

Line wrapping

9680dc4

jnothman merged commit 8056d63 into scikit-learn:master Dec 15, 2016

sergeyf pushed a commit to sergeyf/scikit-learn that referenced this pull request Feb 28, 2017

[MRG+2] ENH add n_jobs to make_union through kwargs (scikit-learn#8031)

e180ce6

Przemo10 mentioned this pull request Mar 17, 2017

update fork (#1) #8606

Closed

Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017

[MRG+2] ENH add n_jobs to make_union through kwargs (scikit-learn#8031)

f72c27d

NelleV pushed a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017

[MRG+2] ENH add n_jobs to make_union through kwargs (scikit-learn#8031)

aa50033

paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017

[MRG+2] ENH add n_jobs to make_union through kwargs (scikit-learn#8031)

cc793a1

maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

[MRG+2] ENH add n_jobs to make_union through kwargs (scikit-learn#8031)

9a94e3d

		@@ -418,6 +418,14 @@ def test_make_union():
		assert_equal(transformers, (pca, mock))


		def test_make_union_kwargs():

Uh oh!

[MRG+2] chore(make_union): add n_jobs to make_union through kwargs #8031

[MRG+2] chore(make_union): add n_jobs to make_union through kwargs #8031

Uh oh!

Conversation

alexandercbooth commented Dec 10, 2016

Uh oh!

jnothman 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!

amueller commented Dec 12, 2016

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!

jnothman commented Dec 15, 2016 via email

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

alexandercbooth commented Dec 15, 2016

Uh oh!

jnothman commented Dec 15, 2016

Uh oh!

alexandercbooth commented Dec 15, 2016

Uh oh!

jnothman commented Dec 15, 2016

Uh oh!

alexandercbooth commented Dec 15, 2016

Uh oh!

Uh oh!