mlr-org / mlr3learners

Recommended learners for mlr3
https://mlr3learners.mlr-org.com
GNU Lesser General Public License v3.0
91 stars 16 forks source link

Learner inheritance #80

Closed adibender closed 3 years ago

adibender commented 4 years ago

Some learners for differnt taks are very similar except for some changes in default arguments and possibly transformation of the target, e.g. LearnerRegrXgboost, LearnerClassifXgboost and LearnerSurvXgboost. Currently, lots of code is duplicated with minor changes. If something changes in one of them, the other learners are not updated, which leads to (unexpectedly) different behaviour.

@berndbischl suggested to ignore this for now

pat-s commented 4 years ago

If something changes in one of them, the other learners are not updated, which leads to (unexpectedly) different behaviour.

What should be updated in detail? Params most often apply to all variants.

Since there is a limited number of possible variations, I would be fine with code duplication here. The same applies for classif & regr. I guess we should have accounted for this way earlier (if at all).

Creating a super class and then inheriting takes way more time than just c/p for the survival learners.

In addition (and this is a valid point), I would like to see unique "Paramtests" for classif/regr/survial specifically listing params only available for the specific type.

Looking at the general questions where learners should live, I'd say

mllg commented 3 years ago

I agree that code duplication is problem here. But as we agreed to put learners from the same package into the same extension package, we can just avoid some duplication with (internal) helper functions, without inheritance.