-
-
Notifications
You must be signed in to change notification settings - Fork 309
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
Comments
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. |
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 |
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.
The text was updated successfully, but these errors were encountered: