MNT Clean-up deprecations for 1.7: sample_weight as positional arg when not consumed #31119
Conversation
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
| sample_weight = _check_sample_weight(sample_weight, X, dtype=None) | ||
| fit_params["sample_weight"] = sample_weight | ||
|
|
There was a problem hiding this comment.
The allow suggestion didn't work here but I removed this validation because bagging doesn't consume sample_weight. So it should delegate validation to the underlying estimator.
Otherwise, if it turns out that bagging should be a consumer, we'll need to put back sample_weight as explicit param, right ?
There was a problem hiding this comment.
+1 to keep sample_weight as explicit param for this release, as indeed we will need them for a PR fixing sample weight in Bagging estimators.
There was a problem hiding this comment.
I don't understand how that affects your work, can you elaborate?
There was a problem hiding this comment.
It's for #31165 where we use sample_weight for drawing the samples instead of passing them to the subestimators. Bagging is then is a consumer of sample_weight, and consumer-only as it should not pass them to underlying estimators.
There was a problem hiding this comment.
Oh interesting. I'd say we can merge this one, and your PR can add it as an explicit parameter. Nothing changes for the user's code, since they can pass sample_weight=sample_weight regardless of it being a part of kwargs or an explicit parameter.
|
Alright so following an irl discussion and this #31119 (comment), I've kept sample_weight as an explicit parameter for This PR is ready for a final review imo |
| sample_weight = _check_sample_weight(sample_weight, X, dtype=None) | ||
| fit_params["sample_weight"] = sample_weight | ||
|
|
There was a problem hiding this comment.
I don't understand how that affects your work, can you elaborate?
…en not consumed (scikit-learn#31119) Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com> Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
Removed deprecated
sample_weightas positional arg in estimators when it's only routed.I haven't removed it from
RANSACyet because the fact that it's only routing sample weight is probably a bug and it should also be a consumer, see #15836. ping @ogrisel for confirmation. In which case,sample_weightshould remain an explicit parameter, right ? ping @adrinjalali