mlr-org / mlr3extralearners

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

add discrete method for survreg to return Matdist #330

Closed RaphaelS1 closed 9 months ago

RaphaelS1 commented 9 months ago

Tests pass locally, first part of #41

@jemus42 this will fix speed and size issues you're seeing in parametric

bblodfon commented 9 months ago

Tests should pass now (expect some lightgbm ones?).

There is still the issue though when the test set is a bit smaller, eg. 10 observations from the rat dataset, there we get slightly different probabilities using discrete = TRUE (new option) vs FALSE (old option). Can easily be replicated by adding predict(task, rows =...) to the new tests. Maybe not an issue after the conversion? I really don't know. @RaphaelS1 what do you think?

RaphaelS1 commented 9 months ago

Tests should pass now (expect some lightgbm ones?).

There is still the issue though when the test set is a bit smaller, eg. 10 observations from the rat dataset, there we get slightly different probabilities using discrete = TRUE (new option) vs FALSE (old option). Can easily be replicated by adding predict(task, rows =...) to the new tests. Maybe not an issue after the conversion? I really don't know. @RaphaelS1 what do you think?

You can't do ML on 10 rows, so I don't really care about this edge case

bblodfon commented 9 months ago

@sebffischer any comments from your side? The issue Raphael commented above has to do with sampling from discrete vs continuous distributions so in general is not an issue and we discussed about it.

sebffischer commented 9 months ago

Sorry I am currently sick and will look at it next monday

sebffischer commented 9 months ago

please also update the NEWS.md file

RaphaelS1 commented 9 months ago

@sebffischer @bblodfon thanks for reviews and help, can we merge?