Skip to content

[MRG] Use base.is_classifier instead of isinstance#9482

Merged
lesteve merged 1 commit intoscikit-learn:masterfrom
minghui-liu:is_classifier
Aug 8, 2017
Merged

[MRG] Use base.is_classifier instead of isinstance#9482
lesteve merged 1 commit intoscikit-learn:masterfrom
minghui-liu:is_classifier

Conversation

@minghui-liu
Copy link
Copy Markdown
Contributor

@minghui-liu minghui-liu commented Aug 2, 2017

Reference Issue

Fixes #9475

What does this implement/fix? Explain your changes.

change isinstance(estimator, ClassifierMixin) to use base.is_classifier() and isinstance(estimator, RegressorMixin) to use base.is_regressor()

Any other comments?

There are also occurrences of isinstance(estimator, TransformerMixin) and isinstance(estimator, ClusterMixin). I would like to add base.is_transformer() and base.is_cluster() methods and use them instead.

@minghui-liu minghui-liu changed the title use base.is_classifier instead of isinstance [WIP] use base.is_classifier instead of isinstance Aug 2, 2017
@minghui-liu minghui-liu changed the title [WIP] use base.is_classifier instead of isinstance [WIP] Use base.is_classifier instead of isinstance Aug 2, 2017
@jnothman
Copy link
Copy Markdown
Member

jnothman commented Aug 2, 2017

It's not essential to do this to check self, but I think this is good. LGTM

@jnothman
Copy link
Copy Markdown
Member

jnothman commented Aug 2, 2017

Sorry is there a reason you marked it WIP? What is incomplete?

@minghui-liu
Copy link
Copy Markdown
Contributor Author

minghui-liu commented Aug 2, 2017

I plan to add base.is_transformer() and base.is_cluster() and use them instead of isinstance(estimator, TransformerMixin) and isinstance(estimator, ClusterMixin). But I can do that in another PR.

@minghui-liu minghui-liu changed the title [WIP] Use base.is_classifier instead of isinstance [MRG] Use base.is_classifier instead of isinstance Aug 2, 2017
TomDLT
TomDLT approved these changes Aug 3, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@TomDLT TomDLT left a comment

Choose a reason for hiding this comment

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

LGTM

lesteve
lesteve reviewed Aug 3, 2017
View reviewed changes
sklearn/tree/tree.py Outdated
from ..base import BaseEstimator
from ..base import ClassifierMixin
from ..base import RegressorMixin
from ..base \
Copy link
Copy Markdown
Member

@lesteve lesteve Aug 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Please refrain from making changes unless there is a very strong reason behind it. This adds noise to the diff and this unnecessary messiness is quite annoying as a reviewer.

If your editor reorganises imports automatically, find a way to disable it.

Apart from this LGTM.

@amueller
Copy link
Copy Markdown
Member

amueller commented Aug 4, 2017

I don't think we should add other issomething methods, I need to get moving with the estimator tags instead.

@amueller
Copy link
Copy Markdown
Member

amueller commented Aug 4, 2017

LGTM, too. But I particularly dislike the \ for continuation.

@lesteve
Copy link
Copy Markdown
Member

lesteve commented Aug 8, 2017

There was a missing isinstance(..., ClassifierMixin) so I pushed an update directly in this branch (also I reverted the controversial cosmetic changes).

I'll merge this one when the CIs are green.

@lesteve
Copy link
Copy Markdown
Member

lesteve commented Aug 8, 2017

OK Travis is green, let's merge this one! Thanks @minghui-liu!

@lesteve lesteve merged commit ffe9866 into scikit-learn:master Aug 8, 2017
jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Aug 8, 2017
@minghui-liu @jnothman
Use base.is_classifier instead instead of isinstance (scikit-learn#9482)
6c6d6a2
@minghui-liu
Copy link
Copy Markdown
Contributor Author

minghui-liu commented Aug 8, 2017

Thank you, @lesteve

@minghui-liu minghui-liu deleted the is_classifier branch August 8, 2017 16:25
yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Aug 12, 2017
@yarikoptic
Merge tag '0.19.0' into releases
445af38 
Release 0.19.0

* tag '0.19.0': (99 commits)
  DOC one more version issue in doc
  skip docstring tests because not useful to users and has some issues
  deprecation of n_components happened in 0.19 not 0.18 (scikit-learn#9527)
  sync whatsnew with master so I'm less confused
  DOC more navigation links
  DOC a note on data leakage and pipeline (scikit-learn#9510)
  DOC set release date to Friday
  DOC Update news and menu for 0.19 release
  DOC list of contributors to 0.19
  DOC Change release date to Thursday
  DOC Remove some whitespace from what's new
  Update what's new for recent changes
  Use base.is_classifier instead instead of isinstance (scikit-learn#9482)
  Fix safe_indexing with read-only indices (scikit-learn#9507)
  [MRG+1] add scorer based on explained_variance_score (scikit-learn#9259)
  fix wrong assert in test_validation (scikit-learn#9480)
  [MRG+1] FIX Add missing mixins to ClassifierChain (scikit-learn#9473)
  Bring last code block in line with the image. (scikit-learn#9488)
  FIX Pass sample_weight as kwargs in VotingClassifier (scikit-learn#9493)
  FIX Incorrent implementation of noise_variance_ in PCA._fit_truncated (scikit-learn#9108)
  ...
yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Aug 12, 2017
@yarikoptic
Merge branch 'releases' into dfsg
8f9558b 
* releases: (99 commits)
  DOC one more version issue in doc
  skip docstring tests because not useful to users and has some issues
  deprecation of n_components happened in 0.19 not 0.18 (scikit-learn#9527)
  sync whatsnew with master so I'm less confused
  DOC more navigation links
  DOC a note on data leakage and pipeline (scikit-learn#9510)
  DOC set release date to Friday
  DOC Update news and menu for 0.19 release
  DOC list of contributors to 0.19
  DOC Change release date to Thursday
  DOC Remove some whitespace from what's new
  Update what's new for recent changes
  Use base.is_classifier instead instead of isinstance (scikit-learn#9482)
  Fix safe_indexing with read-only indices (scikit-learn#9507)
  [MRG+1] add scorer based on explained_variance_score (scikit-learn#9259)
  fix wrong assert in test_validation (scikit-learn#9480)
  [MRG+1] FIX Add missing mixins to ClassifierChain (scikit-learn#9473)
  Bring last code block in line with the image. (scikit-learn#9488)
  FIX Pass sample_weight as kwargs in VotingClassifier (scikit-learn#9493)
  FIX Incorrent implementation of noise_variance_ in PCA._fit_truncated (scikit-learn#9108)
  ...
yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Aug 12, 2017
@yarikoptic
Merge branch 'dfsg' into debian
c9e232e 
* dfsg: (99 commits)
  DOC one more version issue in doc
  skip docstring tests because not useful to users and has some issues
  deprecation of n_components happened in 0.19 not 0.18 (scikit-learn#9527)
  sync whatsnew with master so I'm less confused
  DOC more navigation links
  DOC a note on data leakage and pipeline (scikit-learn#9510)
  DOC set release date to Friday
  DOC Update news and menu for 0.19 release
  DOC list of contributors to 0.19
  DOC Change release date to Thursday
  DOC Remove some whitespace from what's new
  Update what's new for recent changes
  Use base.is_classifier instead instead of isinstance (scikit-learn#9482)
  Fix safe_indexing with read-only indices (scikit-learn#9507)
  [MRG+1] add scorer based on explained_variance_score (scikit-learn#9259)
  fix wrong assert in test_validation (scikit-learn#9480)
  [MRG+1] FIX Add missing mixins to ClassifierChain (scikit-learn#9473)
  Bring last code block in line with the image. (scikit-learn#9488)
  FIX Pass sample_weight as kwargs in VotingClassifier (scikit-learn#9493)
  FIX Incorrent implementation of noise_variance_ in PCA._fit_truncated (scikit-learn#9108)
  ...
paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
@minghui-liu @paulha
Use base.is_classifier instead instead of isinstance (scikit-learn#9482)
77cf533
AishwaryaRK pushed a commit to AishwaryaRK/scikit-learn that referenced this pull request Aug 29, 2017
@minghui-liu @AishwaryaRK
Use base.is_classifier instead instead of isinstance (scikit-learn#9482)
9867ca4
maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
@minghui-liu @maskani-moh
Use base.is_classifier instead instead of isinstance (scikit-learn#9482)
5c01a4f
jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017
@minghui-liu
Use base.is_classifier instead instead of isinstance (scikit-learn#9482)
2b45158
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lesteve lesteve lesteve left review comments

+1 more reviewer

@TomDLT TomDLT TomDLT approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

estimator_checks use isinstance(estimator, ClassifierMixin)

5 participants

@minghui-liu @jnothman @amueller @lesteve @TomDLT