Skip to content

[MRG + 1] Function transformer rebase#5059

Merged
larsmans merged 5 commits intoscikit-learn:masterfrom
amueller:function_transformer_rebase
Aug 3, 2015
Merged

[MRG + 1] Function transformer rebase#5059
larsmans merged 5 commits intoscikit-learn:masterfrom
amueller:function_transformer_rebase

Conversation

@amueller
Copy link
Copy Markdown
Member

@amueller amueller commented Jul 30, 2015

Rebase of #4798. Should be good to go.

@amueller amueller force-pushed the function_transformer_rebase branch 2 times, most recently from 1934501 to 70d31b0 Compare July 30, 2015 16:48
@amueller
Copy link
Copy Markdown
Member Author

amueller commented Jul 30, 2015

Maybe @larsmans wants to review?

jnothman
jnothman reviewed Aug 3, 2015
View reviewed changes
sklearn/preprocessing/function_transformer.py Outdated
Copy link
Copy Markdown
Member

@jnothman jnothman Aug 3, 2015

Choose a reason for hiding this comment

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

Nitpick: This should really check for self.func is None. While obscure, it's possible for an object to be boolean False and still callable.

@amueller
Copy link
Copy Markdown
Member Author

amueller commented Aug 3, 2015

Btw, while I'm fine with merging this as-is, I actually prefered the version with *args, **kwargs.

Joe Jevnik and others added 4 commits August 3, 2015 11:45
@amueller
ENH: Adds CallableTransformer
cb0916c 
CallableTransformer allows a user to convert a standard python callable
into a transformer for use in a Pipeline.
@amueller
ENH: Renames CallableTransformer -> FunctionTransformer.
9d14f73 
Makes `pass_y` an argument to FunctionTransformer to indicate that the
labels should be passed to the wrapped function.
@amueller
COMPAT: Makes test_function_transformer py2 compatible.
ba44e7b
@larsmans @amueller
DOC expand FunctionTransform docstring
bf8b91f
@amueller amueller force-pushed the function_transformer_rebase branch from 70d31b0 to 73532be Compare August 3, 2015 15:48
@amueller amueller force-pushed the function_transformer_rebase branch from 73532be to a2a2d83 Compare August 3, 2015 16:59
larsmans
larsmans reviewed Aug 3, 2015
View reviewed changes
sklearn/preprocessing/tests/test_function_transformer.py
Copy link
Copy Markdown
Member

@larsmans larsmans Aug 3, 2015

Choose a reason for hiding this comment

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

Test should use public import (full path). In fact, I think the module should be called _function_transformer so people don't import it directly.

Copy link
Copy Markdown
Member Author

@amueller amueller Aug 3, 2015

Choose a reason for hiding this comment

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

good point.

larsmans added a commit that referenced this pull request Aug 3, 2015
@larsmans
Merge pull request #5059 from amueller/function_transformer_rebase
3c6cf99 
ENH add FunctionTransformer
@larsmans larsmans merged commit 3c6cf99 into scikit-learn:master Aug 3, 2015
@amueller
Copy link
Copy Markdown
Member Author

amueller commented Aug 3, 2015

Thanks @larsmans. Are you making the module private or should I?

@larsmans
Copy link
Copy Markdown
Member

larsmans commented Aug 3, 2015

I'm on it.

@amueller
Copy link
Copy Markdown
Member Author

amueller commented Aug 3, 2015

Thanks. We also need a whatsnew.

@amueller amueller mentioned this pull request Aug 3, 2015
Closed
@larsmans
Copy link
Copy Markdown
Member

larsmans commented Aug 3, 2015

b09afb0

@amueller
Copy link
Copy Markdown
Member Author

amueller commented Aug 3, 2015

👍

@amueller
Copy link
Copy Markdown
Member Author

amueller commented Aug 3, 2015

🍻 indeed

@amueller amueller mentioned this pull request Aug 4, 2015
Closed
@amueller amueller deleted the function_transformer_rebase branch May 19, 2017 20:45
@glemaitre glemaitre mentioned this pull request Aug 21, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@amueller @larsmans @jnothman