Skip to content
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

[MRG + 3] OneVsRestClassifier: don't expose predict_proba and decision_function if base estimator doesn't support them #7812

Merged
merged 3 commits into from Nov 7, 2016

Conversation

Projects
None yet
4 participants
@kmike
Copy link
Contributor

kmike commented Nov 2, 2016

Currently OneVsRestClassifier has predict_proba and decision_function methods even if base estimator doesn't have them. This makes it harder to check if an estimator supports predict_proba or not because it is not enough to run hasattr(estimator, 'predict_proba'), one have to special-case OneVsRestClassifier.

In this PR it is changed, so that OneVsRestClassifier is more similar to Pipeline: if base estimator doesn't provide predit_proba or decision_function then OneVsRestClassifier also doesn't provide them.

It is not 100% backwards compatible: AttributeError is raised earlier, on ovr.decision_function / ovr.predict_proba access, not when these methods are called.

OneVsRestClassifier: don't expose predict_proba and decision_function
methods if they are not supported by base estimator.

@kmike kmike changed the title OneVsRestClassifier: don't expose predict_proba and decision_function OneVsRestClassifier: don't expose predict_proba and decision_function if base estimator doesn't support them Nov 2, 2016

decision_only = OneVsRestClassifier(svm.SVR()).fit(X_train, Y_train)
assert_raises(AttributeError, decision_only.predict_proba, X_test)
assert not hasattr(decision_only, 'predict_proba')

This comment has been minimized.

Copy link
@amueller

amueller Nov 2, 2016

Member

these are pytest style asserts, while we are still using nosetests. Maybe use "assert_true" and "assert_false" until we transition?

This comment has been minimized.

Copy link
@kmike

kmike Nov 2, 2016

Author Contributor

a good point; it seems I got too used to pytest asserts :)

@amueller

This comment has been minimized.

Copy link
Member

amueller commented Nov 2, 2016

I'm +1 for this change and it LGTM.
It does sort-of change API but I'd argue the previous behavior was not great.

@amueller amueller changed the title OneVsRestClassifier: don't expose predict_proba and decision_function if base estimator doesn't support them [MRG + 1] OneVsRestClassifier: don't expose predict_proba and decision_function if base estimator doesn't support them Nov 2, 2016

@jnothman

This comment has been minimized.

Copy link
Member

jnothman commented Nov 2, 2016

In #7155 we discovered that there are problems determining predict_proba capability prior to fit. Should we solve that here too, as in #7401?

@kmike

This comment has been minimized.

Copy link
Contributor Author

kmike commented Nov 2, 2016

@jnothman a nice catch; I thought about a fallback when writing this PR, but failed to come up with an example where checking self.estimator doesn't work. I'll fix that.

@kmike

This comment has been minimized.

Copy link
Contributor Author

kmike commented Nov 3, 2016

Does it look better now?

@jnothman

This comment has been minimized.

Copy link
Member

jnothman commented Nov 5, 2016

LGTM. If someone (@raghavrv? @ogrisel) wants to confirm these latest tweaks, we can merge. Please add a what's new, though perhaps @amueller will decide that this can squeeze into 0.18.1

@jnothman jnothman changed the title [MRG + 1] OneVsRestClassifier: don't expose predict_proba and decision_function if base estimator doesn't support them [MRG+2] OneVsRestClassifier: don't expose predict_proba and decision_function if base estimator doesn't support them Nov 5, 2016

@raghavrv

This comment has been minimized.

Copy link
Member

raghavrv commented Nov 7, 2016

though perhaps @amueller will decide that this can squeeze into 0.18.1

+1 for 0.18.1 as this is kind of a bug fix right?


# Estimator which can get predict_proba enabled after fitting
gs = GridSearchCV(svm.SVC(probability=False),
param_grid={'probability': [True]})

This comment has been minimized.

Copy link
@raghavrv

raghavrv Nov 7, 2016

Member

Thanks for testing this.

@raghavrv raghavrv changed the title [MRG+2] OneVsRestClassifier: don't expose predict_proba and decision_function if base estimator doesn't support them [MRG + 2] OneVsRestClassifier: don't expose predict_proba and decision_function if base estimator doesn't support them Nov 7, 2016

@raghavrv raghavrv changed the title [MRG + 2] OneVsRestClassifier: don't expose predict_proba and decision_function if base estimator doesn't support them [MRG + 3] OneVsRestClassifier: don't expose predict_proba and decision_function if base estimator doesn't support them Nov 7, 2016

@raghavrv raghavrv merged commit 3b1541b into scikit-learn:master Nov 7, 2016

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@raghavrv

This comment has been minimized.

Copy link
Member

raghavrv commented Nov 7, 2016

Thanks @kmike

sergeyf added a commit to sergeyf/scikit-learn that referenced this pull request Feb 28, 2017

[MRG + 3] OneVsRestClassifier: don't expose predict_proba and decisio…
…n_function if base estimator doesn't support them (scikit-learn#7812)

* OneVsRestClassifier: don't expose predict_proba and decision_function
  methods if they are not supported by base estimator.
* TST use nose-style asserts
* handle a case where classifier get predict_proba method only after .fit

afiodorov added a commit to unravelin/scikit-learn that referenced this pull request Apr 25, 2017

[MRG + 3] OneVsRestClassifier: don't expose predict_proba and decisio…
…n_function if base estimator doesn't support them (scikit-learn#7812)

* OneVsRestClassifier: don't expose predict_proba and decision_function
  methods if they are not supported by base estimator.
* TST use nose-style asserts
* handle a case where classifier get predict_proba method only after .fit

Sundrique added a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017

[MRG + 3] OneVsRestClassifier: don't expose predict_proba and decisio…
…n_function if base estimator doesn't support them (scikit-learn#7812)

* OneVsRestClassifier: don't expose predict_proba and decision_function
  methods if they are not supported by base estimator.
* TST use nose-style asserts
* handle a case where classifier get predict_proba method only after .fit

NelleV added a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017

[MRG + 3] OneVsRestClassifier: don't expose predict_proba and decisio…
…n_function if base estimator doesn't support them (scikit-learn#7812)

* OneVsRestClassifier: don't expose predict_proba and decision_function
  methods if they are not supported by base estimator.
* TST use nose-style asserts
* handle a case where classifier get predict_proba method only after .fit

paulha added a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017

[MRG + 3] OneVsRestClassifier: don't expose predict_proba and decisio…
…n_function if base estimator doesn't support them (scikit-learn#7812)

* OneVsRestClassifier: don't expose predict_proba and decision_function
  methods if they are not supported by base estimator.
* TST use nose-style asserts
* handle a case where classifier get predict_proba method only after .fit

maskani-moh added a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

[MRG + 3] OneVsRestClassifier: don't expose predict_proba and decisio…
…n_function if base estimator doesn't support them (scikit-learn#7812)

* OneVsRestClassifier: don't expose predict_proba and decision_function
  methods if they are not supported by base estimator.
* TST use nose-style asserts
* handle a case where classifier get predict_proba method only after .fit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.