MNT Deprecates _estimator_type and replaces by a estimator tag

[alfaro96]

Reference Issues/PRs

Closes #16469.

What does this implement/fix? Explain your changes.

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

[rth]

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]

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]

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

[rth]

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

[alfaro96]

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]

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]

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]

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.

[amueller]

Sorry for coming late to the party, but I think this should be an actual backward compatible deprecation. Otherwise we're changing the results of downstream libraries without warning. This is private in the sense that it's hidden from users, but it's actually part of the contract with downstream libraries and we can't just change their results imho.

[alfaro96]

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

[jnothman]

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

[alfaro96]

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]

Do we have a test for this logic?

[alfaro96]

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

[alfaro96]

The remaining tests should pass when #18614 is merged.