-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
FIX CalibratedClassifierCV
should not ignore sample_weight
if estimator does not support it
#21143
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
Conversation
Now that I am looking at the calibration code, it seems that we intend to raise a Warning (which I ignore while doing my primary tests). @lucyleeow do you remember why was not it controversial to not fit a model discarding |
I think the intention when I refactored this function was to keep all functionality the same, and any fixes/changes to be done afterwards, separately. (not that I can remember any fixes..!) Looking through git blame, it seems that the warning about when sample weight is ignored is added here: 70d49de And it seems this ignoring of sample weight originates from the start? ecfc93d: for train, test in cv:
this_estimator = clone(self.base_estimator)
if sample_weight is not None and \
"sample_weight" in inspect.getargspec(
this_estimator.fit)[0]:
this_estimator.fit(X[train], y[train],
sample_weight[train])
else:
this_estimator.fit(X[train], y[train]) |
Thanks @lucyleeow for the insights. I am doubting that this is a good strategy, though. I will raise this issue in the next dev meeting then. |
CalibratedClassifierCV
should not ignore sample_weight
if estimator does not support it
I am not sure if passing Maybe the best way to do would be to consider 2 datasets:
Intuitively we would like that calling
We could have a similar test to check that that dropping a sample is equivalent to setting it a weight of 0. There is a common test for this latter semantics but it is XFAILing for https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/calibration.py#L437-L440 So I think we should at least add dedicated tests for For cases that are not working, we should make sure that we include them as part of the PR prototype for SLEP006 on meta-data routing, e.g. as part of #20350. Whether or not we should raise |
Thinking a bit more about this this might be challenging to test with a limited computational budget because the cross-validation strategy might be non-deterministic and the "in expectation" would require a statistical test. To simplifiy the problem we could make sure that we run this test with a simplistic, deterministic CV loop (simple 3 or 5-Fold CV without shuffling or stratification) and put the duplicated samples and sample with weight 2 in the same position (e.g. in the last CV fold in both cases. Same strategy could be adapted to to check the 0 weight / sample drop equivalence. |
I was indeed starting to make a 2-fold cross-validation with iris (only 2 first class) where it would be easy to check the underlying weight of the classifier and the parameters of the calibrator to understand exactly what we are doing with the weight. |
We can postpone this PR until we have proper dispatching with sample props |
Solved by using meta-data routing. |
Partially addressed #21134
Forces to raise an error with
Pipeline
included in meta-estimators to not ignore silentlysample_weight
.In the future, we should address #18159 and the test should not raise an error and delegate the weights to the right estimator in the
Pipeline
.