mlr-org / mlr3extralearners

Extra learners for use in mlr3.
https://mlr3extralearners.mlr-org.com/
90 stars 50 forks source link

Allow to use a custom mboost Family in glmboost regression learner #62

Closed JaGarRod closed 3 years ago

JaGarRod commented 3 years ago

Currently regr.glmboost learner has a parameter family which is of factor type and has to be chosen among 7 predetermined families. Can we introduce support to add a custom family as a parameter?

I would be happy to contribute if some guidance is provided.

To make it backwards compatible, should we create a customFamily parameter that depends on family parameter being FALSE or empty or "custom"?

RaphaelS1 commented 3 years ago

Thanks you're right this is a discrepancy. I'd greatly welcome your contributions, and it's quite easy to do actually because you we have this in surv.glmboost so you can just copy/paste!

https://github.com/mlr-org/mlr3extralearners/blob/main/R/learner_mboost_surv_glmboost.R

No requirement to do so but would be great if you could do the same for all of:

  1. https://github.com/mlr-org/mlr3extralearners/blob/main/R/learner_mboost_classif_gamboost.R
  2. https://github.com/mlr-org/mlr3extralearners/blob/main/R/learner_mboost_classif_glmboost.R
  3. https://github.com/mlr-org/mlr3extralearners/blob/main/R/learner_mboost_regr_gamboost.R
  4. https://github.com/mlr-org/mlr3extralearners/blob/main/R/learner_mboost_regr_glmboost.R
JaGarRod commented 3 years ago

I will first try to finish the pull request for regr_glmboost and once that works I will complete the rest when I find some time

JaGarRod commented 3 years ago

To avoid mistakes like mine in /issues/79, I see the following options:

  1. Add a warning whenever a custom.family parameter is set and the familiy parameter is not "custom".
  2. Raise an error instead of a warning in the previous situation.
  3. Overwrite the family parameter with "custom" whenever a custom.family parameter is provided and add a warning to let the user know.
  4. Change the interface all together (this would also need a review of https://github.com/mlr-org/mlr3extralearners/blob/main/R/learner_mboost_surv_glmboost.R as this learner was implemented following that) and woult not be backwards compatible.

I would suggest 2. or 3. @RaphaelS1 , let me know what you think is best and I will update the learner. Afterwards I will continue with the other three.

RaphaelS1 commented 3 years ago

I don't think 4 is feasible but 3 sounds good. Arguably we don't even need a warning because as you demonstrated, one would naturally expect the family to be custom if a custom one is supplied. Would you mind also updating surv as well?

I'd also recommend adding an example to each to demonstrate how to use a custom family

JaGarRod commented 3 years ago

See #82