-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
TST check equivalence sample_weight in CalibratedClassifierCV #21179
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
TST check equivalence sample_weight in CalibratedClassifierCV #21179
Conversation
Since this PR is reusing some work from #20638 please amend a commit of this PR to mark co-authorship with The following should do:
And type as the commit message:
|
"zero sample_weight is not equivalent to removing samples" | ||
"Due to the cross-validation and sample ordering, removing a sample" | ||
" is not strictly equal to putting is weight to zero. Specific unit" | ||
" tests are added for CalibratedClassifierCV specifically." |
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.
Very informative, thanks!
y_pred_with_weights = calibrated_clf_with_weights.predict_proba(X) | ||
y_pred_without_weights = calibrated_clf_without_weights.predict_proba(X) | ||
|
||
assert_allclose(y_pred_with_weights, y_pred_without_weights) |
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.
Nice and much needed new test!
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.
+1 for merging this incremental fix. The full fix will require metadata routing (SLEP 6) but this is already a net improvement.
Co-authored-by: JulienB-78 <jbohne78-github@yahoo.fr>
69b2565
to
2a71374
Compare
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@rth or @lorentzenchr as sample_weight fans, you might be interested in reviewing this one. |
@agramfort as well :) |
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.
LGTM !
thx @glemaitre !
…-learn#21179) Co-authored-by: JulienB-78 <jbohne78-github@yahoo.fr> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: JulienB-78 <jbohne78-github@yahoo.fr> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
…-learn#21179) Co-authored-by: JulienB-78 <jbohne78-github@yahoo.fr> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
related to #21143
This PR looks at the impact of
sample_weight
inCalibratedClassifierCV
.