Skip to content

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

Merged
merged 10 commits into from
Oct 1, 2021

Conversation

glemaitre
Copy link
Member

@glemaitre glemaitre commented Sep 28, 2021

related to #21143

This PR looks at the impact of sample_weight in CalibratedClassifierCV.

  • Add a test that checks the equivalence between having twice the same sample and passing sample weights of 2 for each sample.
  • Add a test that checks the equivalence between removing sample and passing null sample weights.

@glemaitre glemaitre marked this pull request as draft September 28, 2021 12:39
@ogrisel
Copy link
Member

ogrisel commented Sep 29, 2021

Since this PR is reusing some work from #20638 please amend a commit of this PR to mark co-authorship with
"JulienB-78 <jbohne78-github@yahoo.fr>".

The following should do:

git commit --allow-empty

And type as the commit message:

Grant co-authorship to Julien

Co-authored-by: JulienB-78 <jbohne78-github@yahoo.fr>

"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."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very informative, thanks!

@glemaitre glemaitre marked this pull request as ready for review September 29, 2021 16:25
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)
Copy link
Member

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!

Copy link
Member

@ogrisel ogrisel left a 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>
@glemaitre glemaitre force-pushed the is/calibrated_sample_weight branch from 69b2565 to 2a71374 Compare September 29, 2021 16:35
glemaitre and others added 2 commits September 30, 2021 09:55
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@ogrisel
Copy link
Member

ogrisel commented Sep 30, 2021

@rth or @lorentzenchr as sample_weight fans, you might be interested in reviewing this one.

@ogrisel
Copy link
Member

ogrisel commented Sep 30, 2021

@agramfort as well :)

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM !

thx @glemaitre !

@ogrisel ogrisel merged commit 17a788d into scikit-learn:main Oct 1, 2021
@glemaitre glemaitre mentioned this pull request Oct 23, 2021
10 tasks
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Oct 23, 2021
…-learn#21179)


Co-authored-by: JulienB-78 <jbohne78-github@yahoo.fr>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
glemaitre added a commit that referenced this pull request Oct 25, 2021
Co-authored-by: JulienB-78 <jbohne78-github@yahoo.fr>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
…-learn#21179)


Co-authored-by: JulienB-78 <jbohne78-github@yahoo.fr>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants