GroupedPredictor patch
#619
Conversation
| super().__init__( | ||
| estimator=estimator, | ||
| groups=groups, | ||
| shrinkage=None, |
There was a problem hiding this comment.
Forcing shrinkage to be None
| <p align="center"> | ||
| <img src="../_static/meta-models/grouped-model.png" /> | ||
| </p> | ||
|  |
There was a problem hiding this comment.
Do we want to mention the classifier/regressor objects in the docs here maybe?
There was a problem hiding this comment.
Sounds reasonable
| def test_specialized_classes(meta_cls, estimator, context): | ||
| df = load_chicken(as_frame=True) | ||
| with context: | ||
| meta_cls(estimator=estimator, groups="diet").fit(df[["time", "diet"]], df["weight"].astype(int)) |
There was a problem hiding this comment.
Specialized classes raise with wrong estimator type
| (lambda x: x, pytest.raises(ValueError)), | ||
| ], | ||
| ) | ||
| def test_clf_shrinkage(shrinkage, context): |
There was a problem hiding this comment.
GroupedPredictor with a classifier raises if shrinkage is not None now
| Its equivalent to [`GroupedPredictor`][sklego.meta.grouped_predictor.GroupedPredictor] with `shrinkage=None` | ||
| but it is available only for classification models. | ||
|
|
||
| !!! info "New in version 0.7.5" |
There was a problem hiding this comment.
How do you feel about this?
Proof of rendering:
|
@FBruzzesi Any concerns about releasing this right away? |
Not sure if it's better to go 0.7.5 (as in the info admonitions) or to 0.8.0 as it has breaking changes (no more shrinkage for classification). If you want to wait for the |
Description
As discussed in #616, shrinkage was intended for regression problems and even if in some cases it doesn't raise errors, it would yield incorrect results for classification or outlier detection tasks.
This PR:
GroupedPredictordocstringGroupedRegressorandGroupedClassifier- the latter doesn't allow for shrinkage in__init__GroupedEstimatoras we passes 0.7.0Fixes #616
Type of change
Checklist: