Skip to content

feature: add get_feature_names() and tests to FunctionTransformer#6436

Closed
nelson-liu wants to merge 1 commit intoscikit-learn:masterfrom
nelson-liu:add_get_feature_names_to_function_transformer
Closed

feature: add get_feature_names() and tests to FunctionTransformer#6436
nelson-liu wants to merge 1 commit intoscikit-learn:masterfrom
nelson-liu:add_get_feature_names_to_function_transformer

Conversation

@nelson-liu
Copy link
Copy Markdown
Contributor

Adds get_feature_names() to FunctionTransformer as outlined in #6425. I'm not sure if I did this properly -- is the point of this function to maintain compatibility when get_feature_names() is implemented in Pipeline? Seems quite trivial otherwise / maybe I'm missing something.

@jnothman
Copy link
Copy Markdown
Member

Yes, is quite trivial. Probably this and #6431, also trivial cases, should have been done together. (Not that I've looked at the code yet)

F = FunctionTransformer()
feature_names = F.get_feature_names(["a", "b", "c"])
testing.assert_array_equal(['a', 'b', 'c'], feature_names)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Need to test handling where input_features is not given explicitly. Indeed, we may need to decide whether to return None or to construct a default set of feature names is the better option.

@jnothman
Copy link
Copy Markdown
Member

I mean, it's also possible we should be renaming x1 to f(x1). Any opinion, @amueller ?

@nelson-liu
Copy link
Copy Markdown
Contributor Author

@jnothman I agree with the naming scheme of using f(x0), I think it more clearly shows that its the output of the FunctionTransformer.

@nelson-liu nelson-liu force-pushed the add_get_feature_names_to_function_transformer branch 2 times, most recently from 5f82339 to 90c6f20 Compare February 24, 2016 02:55
@nelson-liu
Copy link
Copy Markdown
Contributor Author

@yenchenlin1994 I've squashed my commits, feel free to cherry pick and add it to one of your existing PRs / create a new one.

----------
input_features : list of string, length len(input_features), optional
String names for input features if available. By default,
"x0", "x1", ... "xn_features" is used.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hello @nelson-liu ,
I think doc here need to be modified since it returns None by default?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

oops, you're correct. I was halfway through implementing x0,x1,etc.. when I realized its probably easier to just stay consistent with what you did / what you and @jnothman discussed on the issue page.

@nelson-liu nelson-liu force-pushed the add_get_feature_names_to_function_transformer branch from 90c6f20 to a53f8e4 Compare February 24, 2016 04:53
@nelson-liu
Copy link
Copy Markdown
Contributor Author

Closing in favor of combined pr with other transformers at #6431

@nelson-liu nelson-liu closed this Feb 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants