MNT Deprecates _estimator_type and replaces by a estimator tag #17806

alfaro96 · 2020-07-01T17:06:21Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

This PR deprecates the _estimator_type private attribute, replaced by a newer estimator tag.

rth · 2020-07-01T21:00:46Z

Thanks! There was a very similar PR by either @thomasjpfan or @NicolasHug and there were concerns there about how putting estimator tags could be potentially redundant with determination made by mixins (as far as I remember). I'm not saying it's a concern in this PR but it would be useful to find that discussion.

alfaro96 · 2020-07-02T07:36:03Z

Thanks! There was a very similar PR by either @thomasjpfan or @NicolasHug and there were concerns there about how putting estimator tags could be potentially redundant with determination made by mixins (as far as I remember). I'm not saying it's a concern in this PR but it would be useful to find that discussion.

Thanks for the advice @rth!

I have not found the PR that you mention, but I am +1 about the fact that _estimator_type private attribute or even using the estimator tags is redundant given the information provided by the mixins (ClassifierMixin, RegressorMixin, etc.).

Do you think that should be (or may be) replaced by the determination made with mixins?

rth · 2020-07-03T08:51:53Z

I also cannot find it. Maybe I'm mis-remembering, too many PRs... Anyway, this looks very reasonable to me.

rth · 2020-07-03T08:56:14Z

Please also add estimator_type to the list of estimator tags the documentation https://scikit-learn.org/stable/developers/develop.html#estimator-tags

alfaro96 · 2020-07-03T09:04:19Z

I also cannot find it. Maybe I'm mis-remembering, too many PRs... Anyway, this looks very reasonable to me.

Do not worry about the mis-remembering.

Maybe @thomasjpfan and @NicolasHug could help us to recover (remember) this discussion?

alfaro96 · 2020-07-03T09:10:06Z

Please also add estimator_type to the list of estimator tags the documentation https://scikit-learn.org/stable/developers/develop.html#estimator-tags

Sure, I will include the estimator_type tag to the list of estimator tags (alphabetically sorted, right?).

In fact, I still need to fix an issue regarding some estimators with multiple levels of inheritance (e.g., LogisticRegression).

I will commit these changes as soon as possible!

NicolasHug · 2020-07-03T13:15:59Z

was a very similar PR by either @thomasjpfan or @NicolasHug and there were concerns there about how putting estimator tags could be potentially redundant with determination made by mixins (as far as I remember).

Maybe you were referring to this? #16622 (comment)

I personally don't see how mixins can help when we're dealing with execution-time properties: what about meta-estimators like e.g. GridSearchCV which can be both a regressor and a classifier depending on their base_estimator?

alfaro96 · 2020-07-03T15:03:56Z

was a very similar PR by either @thomasjpfan or @NicolasHug and there were concerns there about how putting estimator tags could be potentially redundant with determination made by mixins (as far as I remember).

Maybe you were referring to this? #16622 (comment)

I personally don't see how mixins can help when we're dealing with execution-time properties: what about meta-estimators like e.g. GridSearchCV which can be both a regressor and a classifier depending on their base_estimator?

I was thinking about using isinstance(estimator, ClassifierMixin) to determine whether an estimator is a classifier (similarly for regressor, outlier, etc.). However, the use isinstance may potentially introduce an undesired overhead.

alfaro96 · 2020-08-17T21:53:59Z

The _estimator_type private property is now deprecated to avoid undesired errors in the downstream libraries.

jnothman · 2020-09-24T23:08:03Z

Is the plan here to get #18143 right before continuing??

alfaro96 · 2020-09-25T06:54:21Z

Is the plan here to get #18143 right before continuing??

Yep, I am waiting for #18143 to get merged before continuing working on this PR. Thus, I can address the comments in #18143 to make easier the review of this PR.

WDYT?

thomasjpfan

The following most likely needs to inherit BaseEstimator for the other test to pass:

scikit-learn/sklearn/metrics/_plot/tests/test_plot_curve_common.py

Lines 56 to 59 in 193670c

    
           class MyClassifier(ClassifierMixin): 
        
               def fit(self, X, y): 
        
                   self.classes_ = [0, 1] 
        
                   return self

thomasjpfan · 2020-10-09T20:29:04Z

sklearn/pipeline.py

+        estimator = self.steps[-1][1]
+        estimator_tags = estimator._get_tags()


Do we have a test for this logic?

I have not seen this logic in the tests for the pipeline module.

doc/glossary.rst

sklearn/feature_selection/tests/test_rfe.py

sklearn/model_selection/tests/test_search.py

sklearn/feature_selection/tests/test_rfe.py

alfaro96 · 2020-10-13T21:02:46Z

The remaining tests should pass when #18614 is merged.

…or_type

adrinjalali · 2024-11-05T13:25:03Z

I missed this PR @alfaro96 , sorry. Did the same thing in #30122

alfaro96 added 2 commits July 1, 2020 19:04

MNT Deprecate _estimator_type and replace by estimator_tags

0412969

MNT Replace _estimator_type attribute in glossary by estimator_type tag

2aead16

github-actions bot added module:feature_selection module:model_selection module:pipeline labels Jul 1, 2020

alfaro96 added 4 commits July 1, 2020 19:13

MNT Replace _estimator_type attribute in develop by estimator_type tag

a907782

FIX Fix lint

d501d1c

FIX Fix lint

f91c369

MNT Minor change

17f581e

alfaro96 changed the title ~~MNT Deprecate _estimator_type and replace by estimator_tags~~ MNT Deprecated _estimator_type and replaced by a estimator tag Jul 1, 2020

alfaro96 changed the title ~~MNT Deprecated _estimator_type and replaced by a estimator tag~~ [WIP] MNT Deprecated _estimator_type and replaced by a estimator tag Jul 1, 2020

alfaro96 marked this pull request as draft July 1, 2020 17:31

alfaro96 changed the title ~~[WIP] MNT Deprecated _estimator_type and replaced by a estimator tag~~ [WIP] MNT Deprecates _estimator_type and replaces by a estimator tag Jul 1, 2020

FIX Remove estimator_type from DEFAULT_TAGS

376231f

alfaro96 added 4 commits July 3, 2020 15:44

MNT Use simpler solution

99766bb

FIX Check if estimator has estimator_type attribute

8158a5f

MNT Use getattr to return a dict if _get_tags is not defined

24c6fe4

FIX Minor changes

2b732e5

alfaro96 added 2 commits July 3, 2020 17:46

FIX Fix changes to make the CI to pass

9d55a16

DOC Add meaning of estimator_type to developing estimators

05fe203

alfaro96 marked this pull request as ready for review July 3, 2020 15:51

alfaro96 changed the title ~~[WIP] MNT Deprecates _estimator_type and replaces by a estimator tag~~ MNT Deprecates _estimator_type and replaces by a estimator tag Jul 3, 2020

alfaro96 added 4 commits August 13, 2020 16:49

MNT Deprecate _estimator_type private property

a1aa05c

FIX Minor changes

2269a30

FIX Minor changes

67ed0dc

FIX Minor changes

1f24a71

alfaro96 added 9 commits October 8, 2020 19:21

MNT Iter

04eed2e

MNT Solve merge conflicts

1e25e50

FIX Add @Property to biclusters_

e386624

FIX Add unwanted removed lines

ffa6756

FIX Iter

2ecf9f9

FIX Iter

67a5332

FIX Fix warning

8f94407

FIX Remove print statement

a9acf2c

FIX Fix tests

36b0bcd

thomasjpfan reviewed Oct 9, 2020

View reviewed changes

alfaro96 added 5 commits October 13, 2020 14:20

CLN Apply suggested changes

41e1f96

Merge branch 'master' into deprecate_estimator_type

5ffb315

FIX Fix linting issue

5aace6c

FIX Fix tests

49859bf

FIX Fix tests

e26433a

alfaro96 added the ci sync label Oct 14, 2020

github-actions bot removed the ci sync label Oct 14, 2020

Merge remote-tracking branch 'upstream/master' into deprecate_estimat…

ea96ef7

…or_type

thomasjpfan mentioned this pull request Nov 9, 2020

Do we have a compelling reason to enforce tags? #18798

Open

Base automatically changed from master to main January 22, 2021 10:52

adrinjalali closed this Nov 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MNT Deprecates _estimator_type and replaces by a estimator tag #17806

MNT Deprecates _estimator_type and replaces by a estimator tag #17806

alfaro96 commented Jul 1, 2020 •

edited

Loading

rth commented Jul 1, 2020 •

edited

Loading

alfaro96 commented Jul 2, 2020 •

edited

Loading

rth commented Jul 3, 2020

rth commented Jul 3, 2020

alfaro96 commented Jul 3, 2020

alfaro96 commented Jul 3, 2020

NicolasHug commented Jul 3, 2020

alfaro96 commented Jul 3, 2020

alfaro96 commented Aug 17, 2020

jnothman commented Sep 24, 2020

alfaro96 commented Sep 25, 2020

thomasjpfan left a comment

thomasjpfan Oct 9, 2020

alfaro96 Oct 13, 2020

alfaro96 commented Oct 13, 2020

adrinjalali commented Nov 5, 2024

	class MyClassifier(ClassifierMixin):
	def fit(self, X, y):
	self.classes_ = [0, 1]
	return self

		estimator = self.steps[-1][1]
		estimator_tags = estimator._get_tags()

MNT Deprecates _estimator_type and replaces by a estimator tag #17806

MNT Deprecates _estimator_type and replaces by a estimator tag #17806

Conversation

alfaro96 commented Jul 1, 2020 • edited Loading

Reference Issues/PRs

What does this implement/fix? Explain your changes.

rth commented Jul 1, 2020 • edited Loading

alfaro96 commented Jul 2, 2020 • edited Loading

rth commented Jul 3, 2020

rth commented Jul 3, 2020

alfaro96 commented Jul 3, 2020

alfaro96 commented Jul 3, 2020

NicolasHug commented Jul 3, 2020

alfaro96 commented Jul 3, 2020

alfaro96 commented Aug 17, 2020

jnothman commented Sep 24, 2020

alfaro96 commented Sep 25, 2020

thomasjpfan left a comment

Choose a reason for hiding this comment

thomasjpfan Oct 9, 2020

Choose a reason for hiding this comment

alfaro96 Oct 13, 2020

Choose a reason for hiding this comment

alfaro96 commented Oct 13, 2020

adrinjalali commented Nov 5, 2024

alfaro96 commented Jul 1, 2020 •

edited

Loading

rth commented Jul 1, 2020 •

edited

Loading

alfaro96 commented Jul 2, 2020 •

edited

Loading