fix Sklearnwrapper output for OneHotEncoder and PolynomialFeatures to avoid duplicated features

[datacubeR]

Hi @solegalli,
This is my shot for fixing #489.
After checking the code in detail I think the issue affects not only PolynomialFeatures but also Sklearn OneHotEncoder.
When using SklearnTransformerWrapper + OneHotEncoder I get this:

Which is totally unexpected, since the idea is to replace Categorical Features for OneHotEncoded ones.

After the fix I'm proposing I get this for OneHotEncoder

Which I think is the expected behavior.

For PolynomialFeatures I get the following:

Please note that an OrdinalEncoder was applied to Categorical Variables before applying PolynomialFeatures, otherwise it throws an error since they are not numerical features.

I think these cases could be added as test cases if you think it's OK. But before I would love to get your feedback.

Best,

Alfonso

[solegalli]

Hi @datacubeR

Thanks a lot for the changes.

I do agree that the idea is to replace the original categorical variables by the one hot encoded ones. So I guess, it is OK to have them removed from the dataset as you did.

Originally I thought, if that is the intended functionality, then why not use the OHE from feature engine instead of the one from sklearn? that is why I decided not to drop them. But I think it makes sense to go ahead with your suggestion.

I think it would be great to have those code snippets as tests, and also maybe in the user guide: https://feature-engine.readthedocs.io/en/latest/user_guide/wrappers/Wrapper.html#sklearn-wrapper (I mean examples of wrapping the OHE and the PolynomialFeatures)

would you be able to do that?

thanks a lot!

[solegalli]

PS: I fixed the failing tests in the main branch last week. It was something related to the boxcox with the latest version of scipy. If you sync your main branch and then rebase it to this feature branch, it should get those sorted @datacubeR

thank you!

[datacubeR]

@solegalli I think my fork is up to date, but the failing tests is because they considering adding Features rather than replacing the existing feature for the transformed ones. I will fix those and I'll get back to you.

[datacubeR]

@solegalli do I need to create a separate PR for the examples in the User Guide or I just include those in this one?

[solegalli]

parate PR for the exampl

Here would be good :)

Thank you!

[solegalli]

It's looking good.

I think we need to add a test to corroborate that the method get_feature_names_out returns the original features, without the ones used in the transformation, plus the new ones, when it is called without passing the input_feature parameters.

so tr_wrap.get_feature_names_out() should return = feature_names_in - self.variables_ + new features

Could you add a test for that please? Or is it already there? I mean just for the Poly and OHE

[datacubeR]

Would you please review the wording of the new examples added?

[datacubeR]

This is the test for Polynomial Features

[solegalli]

the test that is missing is something like this:

def test_wrap_polynomial_features_get_features_name_out():
    X = fetch_california_housing(as_frame=True).frame

    varlist = ["MedInc", "HouseAge", "AveRooms", "AveBedrms"]
    tr_wrap = SklearnTransformerWrapper(transformer=PolynomialFeatures(), variables=varlist)
   
    tr_wrap.fit(X)

    assert tr_wrap.get_feature_names_out() == The expected list (all features in the original + new features)
    assert tr_wrap.get_feature_names_out(varlist) == All the new polynomial features
    assert tr_wrap.get_feature_names_out(["Medinc") == the features expected from Medinc

The thing is, given that the logic in get_feature_names_out was changed slightly, I would like to make sure that it still returns what it is expected to return given the input to input_features

Would you be able to change it?

[datacubeR]

@solegalli, I was checking this one, and just to make sure I didn't touch get_feature_names_out. The thing is I'm not sure what is the expected behavior for this one: assert tr_wrap.get_feature_names_out(["Medinc") == the features expected from Medinc.
This one should work even if it was trained on a different pool of features? And the expected output is: Medinc, Medinc^2 and all the interactions that includes Medinc?

Currently, tr_wrap.get_feature_names_out(["Medinc"]) outputs: ValueError: input_features is not equal to feature_names_in_

[solegalli]

Assuming that the transformer was trained as in my example:

    varlist = ["MedInc", "HouseAge", "AveRooms", "AveBedrms"]
    tr_wrap = SklearnTransformerWrapper(transformer=PolynomialFeatures(), variables=varlist)

Then the output of tr_wrap.get_feature_names_out(["Medinc") would be as you say the polynomial combinations that involve MedInc. And this, should come out of the box from the Polynomial Features.

If the transformer was trained like this instead:

    varlist = ["HouseAge", "AveRooms", "AveBedrms"]
    tr_wrap = SklearnTransformerWrapper(transformer=PolynomialFeatures(), variables=varlist)

where MedInc was not part of variables, then the outcome of tr_wrap.get_feature_names_out(["Medinc") should be an error, and I think this should be handled by our class method get_feature_names_out.

[datacubeR]

@solegalli, This issue was a bit more complicated than I thought. Actually I noticed the following things:

• First, the current implementation throws an error because as per Sklearn Documentation input_features needs to be equals to name_features_in when using get_feature_names_out in PolynomialFeatures and OneHotEncoder. See this:

• So adding something like tr_wrap.get_feature_names_out(["Medinc"]) is not valid when trained with more features.

• This means current implementation does not work as expected and cannot be obtained out of the box as mentioned previously.

I implemented a solution when input_features is not None, but I find it not very elegant:

# Get the names of all the new features
added_features = self.transformer_.get_feature_names_out(
                    self.variables_
                )
# Get all the features related to input_features... Sorry for the double for loop
feature_names = []
for feature in added_features:
    for _in in input_features:
        if _in in feature and feature not in feature_names:
            feature_names.append(feature)

# In case of PolynomialFeatures and include_bias is True I need to also retrieve the Bias called '1'
if (
    self.transformer_.__class__.__name__ == "PolynomialFeatures"
    and self.transformer_.include_bias
):
    feature_names = ["1"] + feature_names

The reason why I'm using this double for loop (specially for PolynomialFeatures) is because I need to check all the created features (squared ones, interactions, etc.) related to an input features are included and not repeated. Adittionally in case of include bias, I'm adding '1' as part of the output feature names, I did this because it was the expected behavior for test_get_feature_names_out_polynomialfeature. After a lot of trial an error (breaking a lot of tests) I came up to this solution, but I'm totally open to make it better. Didn't find a better way to implement this with lists. I tried sets, but they don't preserve the order of features, so order of features is a bit messy and difficult to test.

The thing is, if an input_variable is not part of the training variables not error is thrown, so what error should I raise if this happens?

PS: I'm currently passing all the tests locally and I'm getting the results as expected in our discussion, if you accept a solution like this I can push my changes.

Thanks!!

[solegalli]

Hi @datacubeR

Thank you so much for so much detail.

I wonder what the point was in offering input_features as parameter in the PolynomialFeatures , if then you can only pass feature_names_in :/

In this case, I think we do not need to modify the code further or add the additional test.

Sorry, that was my bad. For some reason I thought that you could pass one variable and obtain the derived features.

[datacubeR]

This is the test for OneHotEncoder

[solegalli]

Same as per previous comment, the idea is to just test the functionality of the method, and not in comparison to that of sklearn's.

[solegalli]

Could you replace this sentence by the below:

Even though Feature-engine has its own implementation of OneHotEncoder, you may want to use Scikit-Learn'stransformer in order to access different options, such as drop first Category. In the following example, we show you how to apply Scikit-learn's OneHotEncoder to a subset of categories using the :class:SklearnTransformerWrapper().

[solegalli]

The point of the sklearntransformerwrapper is to apply sklearn transformer only to a subset of variables. In this case, the df has only one variable, so you could just apply the OHE directly.

I think a more relevant example would be to use the entire titanic data, and apply the ohe to just a subset for example pclass and sex.

[datacubeR]

Ok!!

[solegalli]

It would be good to add an image or a printout of the result, for example execute X_train_transform.head() and after that show an image of the final df, or instead print(X_train_transform.head()) and then copy the code output in a code block.

[datacubeR]

Sure

[solegalli]

It would be great if you could shorten the length of the sentence to 88 char (that is, break the sentence over 2 lines).

It reads well, the last 2 words should be: Scikit-Learn's PolynomialFeatures().

[solegalli]

great example, thank you!!

Could you add a 5 row display of the resulting dataframe? like in my previous comment.

[solegalli]

Hey @datacubeR

This is almost ready. Some small changes here and there. Would you be able to have a look?

Thank you!

[solegalli]

Hi @datacubeR

Thank you for looking into the get_feature_names_out() issue. I think we can ignore that request then.

Let me know when this is good to go.

Thanks a lot

[datacubeR]

Hi @solegalli, sorry for the delay. I updated my OS and had to reinstall lot of things. I just pushed all the last observations for your review. Please let me know if everything looks ok.

Best,

Alfonso

[solegalli]

Thank you @datacubeR !!

Great fix.