[MRG] Make CalibratedClassifierCV a MetaEstimator#13575
[MRG] Make CalibratedClassifierCV a MetaEstimator#13575qinhanmin2014 merged 5 commits intoscikit-learn:masterfrom
Conversation
agramfort
left a comment
There was a problem hiding this comment.
it should be the case indeed +1
|
Looking at this comment: #13077 (comment), it seemed that before some tests were not run on |
|
I just added a what's new entry, though I'm not sure whether I should have put the first one (about the MetaEstimator, since it maybe doesn't change anything for the users ?) |
|
Also, about the bug fix, I don't think I need to provide a non-regression test since the test is already "added" thanks to the discovery of the tests for |
doc/whats_new/v0.21.rst
Outdated
| between 0 and 1. | ||
| :issue:`13086` by :user:`Scott Cole <srcole>`. | ||
|
|
||
| - |Enhancement| Made `calibration.CalibratedClassifierCV` inherit from |
There was a problem hiding this comment.
This change isn't user facing. No need to litter the change log with such things IMO.
| warnings.warn("%s does not support sample_weight. Samples" | ||
| " weights are only used for the calibration" | ||
| " itself." % estimator_name) | ||
| sample_weight = check_array(sample_weight, ensure_2d=False) |
There was a problem hiding this comment.
Uhm how it can be a bug since that sample_weight will not be given to the underlying estimator. Do I miss something?
There was a problem hiding this comment.
We sill need to pass sample_weight to _CalibratedClassifier
qinhanmin2014
left a comment
There was a problem hiding this comment.
We don't have a test right? but OK.
|
Thanks for the review @qinhanmin2014 and @glemaitre ! I realized that the what's new entry is in v.0.21, should I revert this PR and move it to 0.22 ? Should I add a test too ? Like check with a 3D array for instance that it works ? |
thanks, I'll push a commit.
Not sure, at least I'm OK to merge without a test. |
|
All right, thanks ! |
See #13485 (comment)
This PR makes CalibratedClassifierCV a MetaEstimator
It's WIP because I think it makes sense to do that, but I still need to understand what will it change, and see what are unexpected consequences, and add non-regression tests if needed.