Skip to content

[MRG] Allow nd array for CalibratedClassifierCV#13485

Merged
jnothman merged 6 commits intoscikit-learn:masterfrom
wdevazelhes:more_tolerance_calibrated_classifier
Apr 4, 2019
Merged

[MRG] Allow nd array for CalibratedClassifierCV#13485
jnothman merged 6 commits intoscikit-learn:masterfrom
wdevazelhes:more_tolerance_calibrated_classifier

Conversation

@wdevazelhes
Copy link
Copy Markdown
Contributor

@wdevazelhes wdevazelhes commented Mar 21, 2019

Reference Issues/PRs

Fixes #13077

What does this implement/fix? Explain your changes.

This allows n-dimensional arrays for CalibratedClassifierCV (for instance, GridSearchCV allows for all kind of arrays, since it's a MetaEstimator and more precise tests are made by the encapsulated estimator itself). See #13077 for more details.

@wdevazelhes
Copy link
Copy Markdown
Contributor Author

wdevazelhes commented Mar 21, 2019

Is there a reason why CalibratedClassifierCV is not a Meta-Estimator ?

scikit-learn/sklearn/calibration.py

Line 31 in 7684298

class CalibratedClassifierCV(BaseEstimator, ClassifierMixin):

@wdevazelhes wdevazelhes changed the title Allow nd array for CalibratedClassifierCV [MRG] Allow nd array for CalibratedClassifierCV Mar 21, 2019
@jnothman
Copy link
Copy Markdown
Member

jnothman commented Mar 21, 2019 via email

@jnothman
Copy link
Copy Markdown
Member

jnothman commented Mar 21, 2019

Please add a non-regression test

@connorbrinton
Copy link
Copy Markdown

connorbrinton commented Apr 1, 2019 •
edited
Loading

@wdevazelhes You might be interested in this PR (by me) which should work with nd-array input (and also supports multi-label probability calibration). It includes tests which you can use if you'd like, to verify that input does not need to be 2d. However, it doesn't include any tests for arrays with more than two dimensions.

@wdevazelhes
Copy link
Copy Markdown
Contributor Author

wdevazelhes commented Apr 4, 2019 •
edited
Loading

@connorbrinton Thanks for the reference ! Great, yes indeed your PR would fix my problem with nd-array :)
I'll do a quick non-regression test that maybe can be easily/quickly merged so that you can merge it in your PR

@wdevazelhes
Copy link
Copy Markdown
Contributor Author

wdevazelhes commented Apr 4, 2019 •
edited
Loading

@jnothman I just added a non-regression test, let me know how it sounds

Is there a reason why CalibratedClassifierCV is not a Meta-Estimator ?

Probably no reason. MetaEstimatorMixin did very little until recently. Now it only does little.

Allright, I'll open a PR for that then
done: #13575

This was referenced Apr 4, 2019
Closed
Merged
@jnothman
Copy link
Copy Markdown
Member

jnothman commented Apr 4, 2019

Please merge latest master

@wdevazelhes
Copy link
Copy Markdown
Contributor Author

wdevazelhes commented Apr 4, 2019

Please merge latest master

Done

@jnothman
Copy link
Copy Markdown
Member

jnothman commented Apr 4, 2019

Please add an |Enhancement| entry to the change log at doc/whats_new/v0.21.rst. Like the other entries there, please reference this pull request with :issue: and credit yourself (and other contributors if applicable) with :user:

@wdevazelhes
Copy link
Copy Markdown
Contributor Author

wdevazelhes commented Apr 4, 2019

Please add an |Enhancement| entry to the change log at doc/whats_new/v0.21.rst. Like the other entries there, please reference this pull request with :issue: and credit yourself (and other contributors if applicable) with :user:

done

@jnothman jnothman merged commit 11d4cf0 into scikit-learn:master Apr 4, 2019
jeremiedbb pushed a commit to jeremiedbb/scikit-learn that referenced this pull request Apr 25, 2019
@wdevazelhes @jeremiedbb
ENH Allow nd array for CalibratedClassifierCV (scikit-learn#13485)
1d03e13
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
@wdevazelhes
ENH Allow nd array for CalibratedClassifierCV (scikit-learn#13485)
de3836b
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
Revert "ENH Allow nd array for CalibratedClassifierCV (scikit-learn#1…
9ad040f 
…3485)"

This reverts commit de3836b.
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
Revert "ENH Allow nd array for CalibratedClassifierCV (scikit-learn#1…
8db0fd8 
…3485)"

This reverts commit de3836b.
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
@wdevazelhes @koenvandevelde
ENH Allow nd array for CalibratedClassifierCV (scikit-learn#13485)
cef6cd0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnothman jnothman jnothman approved these changes

@agramfort agramfort agramfort approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Be more tolerant in check_array for CalibratedClassifierCV

4 participants

@wdevazelhes @jnothman @connorbrinton @agramfort