search

stanfordmlgroup / ngboost

Natural Gradient Boosting for Probabilistic Prediction
Apache License 2.0
1.64k stars 215 forks source link

Add partial fit method to match sklearn API #316

Closed CompRhys closed 1 year ago

CompRhys commented 1 year ago

Currently calling fit on an already trained model simply appends to the base model list. This results in misleading results in CV. I have moved old fit method to partial fit and then explicitly reset the lists in the fit call.

CompRhys commented 1 year ago

@alejandroschuler @ryan-wolbeck

jack-mcivor commented 1 year ago

@CompRhys think you need to run black - probably just removing line 259

CompRhys commented 1 year ago

Also if this partial_fit behaviour was always a bug then perhaps it should be completely removed. Here the aim was to ensure that people who may have designed code around this append operation which differs from the sklearn API would be able to continue to use the model as before by slightly refactoring (provided not using the model inside a sklearn Pipeline)

alejandroschuler commented 1 year ago

Also if this partial_fit behaviour was always a bug then perhaps it should be completely removed. Here the aim was to ensure that people who may have designed code around this append operation which differs from the sklearn API would be able to continue to use the model as before by slightly refactoring (provided not using the model inside a sklearn Pipeline)

It wasn't intended to be used this way but I think it's a good idea to officially support it.

CompRhys commented 1 year ago

This is a fairly major bug imo so would be good to merge when possible

alejandroschuler commented 1 year ago

@ryan-wolbeck