-
Notifications
You must be signed in to change notification settings - Fork 15
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
adding bagged estimator to have better results in terms of monotonicity #82
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @juAlberge, thank you for this PR! Some early thoughts on this :)
Also, adding an example for the competing risk setting would be very nice
self.bagging = bagging # number of models to train | ||
|
||
def fit(self, X, y, times=None): | ||
self.models = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In "sklearn conventions", we set an underscore suffix to identify outputs attributes of the fitting process
self.models = [] | |
self.models_ = [] |
survival_boost_params.pop("bagging") | ||
for i in range(self.bagging): | ||
model = SurvivalBoost( | ||
random_state=self.random_state + i, **survival_boost_params |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to handle the default case when self.random_state is None
|
||
class BaggedSurvivalBoost(BaseEstimator, ClassifierMixin): | ||
def __init__( | ||
# TODO: run a grid search on a few datasets to find good defaults. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 For the TODO
self.models = [] | ||
survival_boost_params = self.get_params() | ||
survival_boost_params.pop("random_state") | ||
survival_boost_params.pop("bagging") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I'm thinking about this, what is your opinion on adding a bagging parameter to SurvivalBoost, instead of defining this new class? This would slightly complicate the internal methods of SurvivalBoost, but we would only have one class to deal with.
new estimator:
BaggedSurvivalBoost
-> argument: bagging: to add the number of SurvivalBoost estimators to be created.
TODO: