[WIP] get_feature_names_out for sklego.preprocessing.

[CarloLepelaars]

This PR solves issue #543 and implements get_feature_names_out for all relevant transformers in sklego.preprocessing (i.e. transformers that do not contain the TrainOnlyTransformerMixin).

Functionality is implemented through adding the _ClassNamePrefixFeaturesOutMixin to the class and making sure self._n_features_out is defined in .fit. This is also generally how scikit-learn implements get_feature_names_out for many of its transformers (Example). Unit tests are added for all new functionality.

P.S. Don't pay attention to the commit history before October 10th. These changes have already been merged into koaning/scikit-lego/main, but is still displayed here as commit history. Will try to fix this. Suggestions to remove these redundant commits from the commit history of this CarloLepelaars/scikit-lego/ fork are welcome.

• Find alternative solution for using _ClassNamePrefixFeaturesOutMixin so it works with scikit-learn on Python 3.7. (Remove Python 3.7. support)
• Add implementation of get_feature_names_out to contributing guidelines so people implement this for each new preprocessor that is not TrainOnly.
• Remove Python 3.7. GitHub Actions pipelines and update Optional dependencies GitHub Actions pipeline to use Python 3.8.
• Add general unit test that checks if get_feature_names_out can be called for all relevant preprocessors and EstimatorTransformer.

[CarloLepelaars]

_ClassNamePrefixFeaturesOutMixin is not available for the scikit-learn version on Python 3.7. 😭
Will find a different solution.

[koaning]

@MBrouns for how long do we want to keep Python3.7 support? I think it's end of life in about 8 months, no?

[MBrouns]

I think I'm okay with dropping 3.7 support. EOL in 8 months is soon enough and if it makes this feature easier to implement I'd say go for it

[CarloLepelaars]

@koaning @MBrouns I added contributing guidelines to the readme. Idea is that contributors of a new preprocessor will make sure to add get_feature_names_out functionality. We might want to consider creating a CONTRIBUTING.MD file like HuggingFace transformer has.

Unfortunately it will take quite some work to get this working for Python 3.7. since in that case we don't have access to the convenient Mixins that implement get_feature_names_out.

If you are fine with dropping Python 3.7. support then the PR seems ready to be merged.

[koaning]

Feel free to remove 3.7 support. Maybe upgrade the github action that checks the optional deps.

[koaning]

Is there a general unit test we might write that can check every component if it's implemented?

[CarloLepelaars]

Don't think I have permissions to update the optional deps actions pipeline and remove Python 3.7. pipelines. Isn't that only available for maintainers?

Is there a general unit test we might write that can check every component if it's implemented?

Good point! Will add a general unit test that checks if get_feature_names_out can be called for all relevant preprocessors (and EstimatorTransformer).

[koaning]

You can change the files in .github in your PR right?

Calmcode has a github actions course that gives more details.

[CarloLepelaars]

Aha of course, forgot I can adjust files in .github. Bumped minimum Python versions from 3.7. to 3.8 in .github and .gitpod.yml. Also added the general unit test.

@koaning The Github Actions pipeline still shows a build (3.7.) job. I don't know why this one still shows up.

[koaning]

I don't know why this one still shows up.

That's a setting that only I can change, but feel free to ignore that.

[koaning]

Please don't remove these. By leaving these comments here, we're conscious of the sklearn tests that we've manually removed.

[CarloLepelaars]

Ok, no problem. Added them back in.

I noticed some print statements in test_patsy_transformer.py. Are they also supposed to be left in?

[koaning]

@MBrouns I'm wondering. Would it be better to have folks import a hidden mixin, or just to have them implement get_feature_names_out themselves? Part of me is uneasy about relying on a non-public part of the sklearn api.

[koaning]

@CarloLepelaars also, seen this?

[MBrouns]

interesting that this is part of the private API. Maybe copy the sklearn implementation into one of our own files then and use that instead for the time being?

[CarloLepelaars]

Fair point, the Mixin seems like the most clean implementation, but agree it is not a good idea to use Mixins from the non-public API. Would be interesting to ask a sklearn core maintainer about this decision. I think we have good arguments to make these kinds of Mixins part of the public API.

@MBrouns, that sounds like a valid option. The tricky thing is that get_feature_names_out in this Mixin also calls a hidden function in sklearn.utils.validation _generate_get_feature_names_out, which calls another hidden function _check_feature_names_in. I'm afraid also copying these over to sklego will pollute the codebase.

@koaning oops, didn't notice the contribution documentation yet. That already looks great! Perhaps we can add a link to here in the "New Features" section of the readme?

[koaning]

That already looks great! Perhaps we can add a link to here in the "New Features" section of the readme?

Sounds good.

[koaning]

Mhm, yeah, this is tricky territory.

If their internal class has many more links to internal objects it'll be even riskier to rely on it.

Open to ideas though, there doesn't seem to be an "obvious" solution here.

[MBrouns]

It seems that there is also a standard test in scikit-learns check_estimator that verifies this feature_names_out behaviour: https://github.com/scikit-learn/scikit-learn/blob/36958fb24/sklearn/utils/estimator_checks.py#L3921

We should probably go over the test cases from estimator_checks to see if any others got added since we made our own version

[CarloLepelaars]

Nice find, @MBrouns! I adjusted the general tests to use check_transformer_get_feature_names_out and check_transformer_get_feature_names_out_pandas.

Unfortunately they both break on the 1st line with TypeError: _get_tags() missing 1 required positional argument: 'self'. Should every estimator have _get_tags implemented? If so, why doesn't it seem to be part of BaseEstimator or TransformerMixin?

[CarloLepelaars]

@koaning As discussed we can keep this PR on hold for now and revisit later. The reason is that even when _ClassNamePrefixFeaturesOutMixin is made part of the public API, it would still restrict people to using the latest version of sklearn.

Some things that we should still consider:

1. The PR for EstimatorTransformer get_feature_names_out for EstimatorTransformer #539 is done, tests are passing (also for Python 3.7) and it does not use the Mixin. Shall we go ahead and merge that one?
2. Do we still want to drop support for Python 3.7.? I can make a separate PR to adjust the Github Actions pipelines, test configs, etc.
3. Perhaps we can still implement get_feature_names_out for sklego.preprocessing ourselves? At least we already have the tests ready. If we implement it this way it is likely that the tests would still pass for Python 3.7.

FYI @MBrouns