Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit the level of class inherintance to two. #616

Open
Morgan-Sell opened this issue Feb 25, 2023 · 2 comments
Open

Limit the level of class inherintance to two. #616

Morgan-Sell opened this issue Feb 25, 2023 · 2 comments

Comments

@Morgan-Sell
Copy link
Collaborator

Is your feature request related to a problem? Please describe.
Currently, some feature-engine classes have more than two levels of inheritance, e.g. classes that are derived from the BaseRecursiveSelector() class.

BaseEstimator(), TransformerMixin(), and GetFeatureNamesOutMixin() --> BaseSelector() --> BaseRecursiveSelector() --> RecursiveFeatureAddition()

This many levels of abstraction significantly increase the difficulty for people to contribute.

Describe the solution you'd like
Let's limit the codebase to two levels of inheritance. There can be multiple base transformers in the _base_transformer section than each other section - e.g., discretisation, encoding, and timeseries - are limited to one base transformer.

Describe alternatives you've considered
feature-engine currently applies an alternative, i.e., more than 2 levels of inheritance, and we see the challenges that it is causing.

@Morgan-Sell
Copy link
Collaborator Author

Morgan-Sell commented May 31, 2023

hi @solegalli,

Before we start to tackle this issue, I thought we should reconfirm we're in agreement with the approach. Of course, we're not making the "10 commandments of feature-engine", but a guideline that should be followed in the vast majority of cases.

Should we remove most of the references to the classes created in the _base_transformers directory? E.g., BaseDiscretizer should be a child of BaseEstimator and TransformerMixin, not BaseNumericalTransformer?

I suspect there will still be some references to the classes created in the _base_transformers directory; however, I expect the frequency to significantly decrease.

I believe we agreed, in general, classes like BaseImputer and BaseDiscretiser will directly inherit Scikit-learn classes like TransformerMixin and BaseEstimator.

@solegalli
Copy link
Collaborator

Hey @Morgan-Sell

I think we definitely need to do something about class inheritance. It is getting difficult to follow. But I'd like to read about software architecture first, and make sure that we are making a change that is going to last long.

Because we are, in essence, going to affect the entire code base. So I want to be sure about the changes that we are going to introduce.

Bottom line, I need to learn more about software design first, lol

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

No branches or pull requests

2 participants