In working on #1989, @freddyaboulton suggested that we make TargetImputer a subclass of SimpleImputer. Unfortunately, when looking for viable components in _get_subclasses, _get_subclasses will only collect classes that are at the very bottom of the hierarchy. For example:
ComponentBase --> Transformer --> SimpleImputer # will only grab SimpleImputer
The issue is, if we subclass SimpleImputer, we will have access to TargetImputer, but SimpleImputer will no longer be found by EvalML as a useable component:
ComponentBase --> Transformer --> SimpleImputer --> TargetImputer
# will only grab TargetImputer, SimpleImputer is no longer a leaf
I believe the original intent is to not grab classes that are not useful (ex: Transformer), but this is a problem if we want to build upon useful components to create new ones.
Original comment here: https://github.com/alteryx/evalml/pull/1989#discussion_r599894996
Options:
1.) Have the TargetImputer use the SimpleImputer.
2.) Don't define TargetImputer component, but instead apply the SimpleImputer to the target.
3.) Delete _get_subclasses() in favor of static list of components. (preferred)
4.) Make _get_subclasses() smarter.
Next step: schedule a discussion to decide how to go about doing this.
My opinion is that we shouldn't change anything unless we want to make inheritance of non base classes a more common pattern in our codebase. We currently don't do that so building a system to allow for that seems low priority to me.
This issue isn't blocking the target imputer or any short-term work we have ahead of us and the current system for identifying subclasses "works". If we really want 3 then that's another story hehe and I'd be down to hear arguments for it but I don't think there's a need to do it.
And if we wanted to leverage the SimpleImputer in the TargetImputer in the future we can use composition over inheritance, which I think is what option 1 is referring to.
Agreed with @freddyaboulton here, also worth noting that we defined _get_subclasses() in the first place because keeping track of a static list of components was a bit tedious / frustrating as we were adding more and more components. If we go for 3, it might make more sense to keep _get_subclasses() but also keep a list of "exceptions" such as the SimpleImputer / TargetImputer case, so that they're included, but other components such as the Transformer are still excluded.
Also okay with the idea of some composition-esque technique if that gets the job done.
@angela97lin Do you think we can close this? I think we agree that we don't need to make changes to _get_subclasses to enable development of the TargetImputer/other components.