Repository: scikit-learn/scikit-learn
Pull request: TST check that binary only classifiers fail on multiclass data
Fixes https://github.com/scikit-learn/scikit-learn/issues/18005

Checks that if the estimator has `tags.classifier_tags.multi_class=False`, then it actually fails.

I would be happier if we had a better error message telling people how to fix their issue though. Not sure what to put there.

cc @chkoar @glemaitre 

Problem statement:
check_estimator and binary_only tag
Shouldn't the `check_estimator` check if you pass a non binary problem for a binary classifier? I mean, if you have an untagged binary estimator that raises an exception the `check_estimator` will fail. On the other hand,  if you have a tagged estimator as `binary_only` the `check_estimator` will pass if you pass a non-binary problem. I would expect to fail in this case.

Hints from issue discussion:
`check_estimator` does not take data as input. It will internally generate data based on the estimator tags.
Yes. I know that. I would expect that it will test a tagged binary classifier that will raise an exception on a non-binary test case.
Currently, if a classifier is tagged binary, it will only be passed binary problems.
For clarification, can you provide a code snippet on the desired behavior?  
As far as I understand, the issue is if one tags `binary_only` by mistake (e.g. because of some class inheritance issue / MRO order problem), the tests corresponding to multi-class classification will be silently skipped and we won't know about it. So it would be good to have some sanity checks for estimator tags as well. Do I understand correctly @chkoar ?
I feel like this is overkill. I interpret the tag as meaning "we should expect failures on non-binary data". But it doesn't mean "it will only work with binary data"
@NicolasHug 
> I feel like this is overkill. 

Probably.

> I interpret the tag as meaning "we should expect failures on non-binary data". But it doesn't mean "it will **only** work with **binary** data"

Basically, I thought exactly the opposite. Since the estimator was tagged with `binary_only` made me to believe that `check_estimator` will expect a failure on non-binary data. Probably I am too sane. 

All started while I was reviewing [this](https://github.com/scikit-learn-contrib/scikit-learn-contrib/issues/44) package where the estimator is binary only.

cc @thomasjpfan @rth 
Interesting, I don't mind a test for this actually. Good point @chkoar .

Commit message: Merge branch 'tests/tag-multiclass-false' of github.com:adrinjalali/scikit-learn into tests/tag-multiclass-false

Patch to review:
diff --git a/doc/whats_new/v1.6.rst b/doc/whats_new/v1.6.rst
index 0c4bf127c0b43..0cb7ac296d27a 100644
--- a/doc/whats_new/v1.6.rst
+++ b/doc/whats_new/v1.6.rst
@@ -428,6 +428,12 @@ Changelog
   calling :func:`utils.validation.check_non_negative`.
   :pr:`29540` by :user:`Tamara Atanasoska <tamaraatanasoska>`.
 
+- |Enhancement| :func:`~sklearn.utils.estimator_checks.check_estimator` and
+  :func:`~sklearn.utils.estimator_checks.parametrize_with_checks` now check and
+  fail if the classifier has the `tags.classifier_tags.multi_class = False` tag
+  but does not fail on multi-class data.
+  :pr:`29874` by `Adrin Jalali`_.
+
 - |FIX| :func:`utils.estimator_checks.parametrize_with_checks` and
   :func:`utils.estimator_checks.check_estimator` now support estimators that
   have `set_output` called on them.
diff --git a/sklearn/utils/estimator_checks.py b/sklearn/utils/estimator_checks.py
index c33a3b6f7dbdf..6aa399c35870b 100644
--- a/sklearn/utils/estimator_checks.py
+++ b/sklearn/utils/estimator_checks.py
@@ -196,6 +196,9 @@ def _yield_classifier_checks(classifier):
     ):
         yield check_class_weight_balanced_linear_classifier
 
+    if not tags.classifier_tags.multi_class:
+        yield check_classifier_not_supporting_multiclass
+
 
 @ignore_warnings(category=FutureWarning)
 def check_supervised_y_no_nan(name, estimator_orig):
@@ -1206,7 +1209,13 @@ def check_dtype_object(name, estimator_orig):
     if hasattr(estimator, "transform"):
         estimator.transform(X)
 
-    with raises(Exception, match="Unknown label type", may_pass=True):
+    err_msg = (
+        "y with unknown label type is passed, but an error with no proper message "
+        "is raised. You can use `type_of_target(..., raise_unknown=True)` to check "
+        "and raise the right error, or include 'Unknown label type' in the error "
+        "message."
+    )
+    with raises(Exception, match="Unknown label type", may_pass=True, err_msg=err_msg):
         estimator.fit(X, y.astype(object))
 
     if not tags.input_tags.string:
@@ -3634,7 +3643,13 @@ def check_classifiers_regression_target(name, estimator_orig):
 
     X = _enforce_estimator_tags_X(estimator_orig, X)
     e = clone(estimator_orig)
-    msg = "Unknown label type: "
+    err_msg = (
+        "When a classifier is passed a continuous target, it should raise a ValueError"
+        " with a message containing 'Unknown label type: ' or a message indicating that"
+        " a continuous target is passed and the message should include the word"
+        " 'continuous'"
+    )
+    msg = "Unknown label type: |continuous"
     if not get_tags(e).no_validation:
         with raises(ValueError, match=msg):
             e.fit(X, y)
@@ -4737,3 +4752,38 @@ def check_do_not_raise_errors_in_init_or_set_params(name, estimator_orig):
 
         # Also do does not raise
         est.set_params(**new_params)
+
+
+def check_classifier_not_supporting_multiclass(name, estimator_orig):
+    """Check that if the classifier has tags.classifier_tags.multi_class=False,
+    then it should raise a ValueError when calling fit with a multiclass dataset.
+
+    This test is not yielded if the tag is not False.
+    """
+    estimator = clone(estimator_orig)
+    set_random_state(estimator)
+
+    X, y = make_classification(
+        n_samples=100,
+        n_classes=3,
+        n_informative=3,
+        n_clusters_per_class=1,
+        random_state=0,
+    )
+    err_msg = (
+        f"The estimator tag `tags.classifier_tags.multi_class` is False for {name} "
+        "which means it does not support multiclass classification. However, it does "
+        "not raise the right `ValueError` when calling fit with a multiclass dataset, "
+        "including the error message 'Only binary classification is supported.' This "
+        "can be achieved by the following pattern:\n\n"
+        "y_type = type_of_target(y, input_name='y', raise_unknown=True)\n"
+        "if y_type != 'binary':\n"
+        "    raise ValueError(\n"
+        "        'Only binary classification is supported. The type of the target ' \n"
+        "        f'is {y_type}.'\n"
+        ")"
+    )
+    with raises(
+        ValueError, match="Only binary classification is supported.", err_msg=err_msg
+    ):
+        estimator.fit(X, y)
diff --git a/sklearn/utils/multiclass.py b/sklearn/utils/multiclass.py
index 71c6b51df73c4..535ddbf6ed697 100644
--- a/sklearn/utils/multiclass.py
+++ b/sklearn/utils/multiclass.py
@@ -226,7 +226,7 @@ def check_classification_targets(y):
         )
 
 
-def type_of_target(y, input_name=""):
+def type_of_target(y, input_name="", raise_unknown=False):
     """Determine the type of data indicated by the target.
 
     Note that this type is the most specific type that can be inferred.
@@ -248,6 +248,12 @@ def type_of_target(y, input_name=""):
 
         .. versionadded:: 1.1.0
 
+    raise_unknown : bool, default=False
+        If `True`, raise an error when the type of target returned by
+        :func:`~sklearn.utils.multiclass.type_of_target` is `"unknown"`.
+
+        .. versionadded:: 1.6
+
     Returns
     -------
     target_type : str
@@ -298,6 +304,17 @@ def type_of_target(y, input_name=""):
     'multilabel-indicator'
     """
     xp, is_array_api_compliant = get_namespace(y)
+
+    def _raise_or_return():
+        """Depending on the value of raise_unknown, either raise an error or return
+        'unknown'.
+        """
+        if raise_unknown:
+            input = input_name if input_name else "data"
+            raise ValueError(f"Unknown label type for {input}: {repr(y)}")
+        else:
+            return "unknown"
+
     valid = (
         (isinstance(y, Sequence) or issparse(y) or hasattr(y, "__array__"))
         and not isinstance(y, str)
@@ -374,17 +391,17 @@ def type_of_target(y, input_name=""):
     # Invalid inputs
     if y.ndim not in (1, 2):
         # Number of dimension greater than 2: [[[1, 2]]]
-        return "unknown"
+        return _raise_or_return()
     if not min(y.shape):
         # Empty ndarray: []/[[]]
         if y.ndim == 1:
             # 1-D empty array: []
             return "binary"  # []
         # 2-D empty array: [[]]
-        return "unknown"
+        return _raise_or_return()
     if not issparse(y) and y.dtype == object and not isinstance(y.flat[0], str):
         # [obj_1] and not ["label_1"]
-        return "unknown"
+        return _raise_or_return()
 
     # Check if multioutput
     if y.ndim == 2 and y.shape[1] > 1:
diff --git a/sklearn/utils/tests/test_estimator_checks.py b/sklearn/utils/tests/test_estimator_checks.py
index 53be77d96e901..2f2e61f259721 100644
--- a/sklearn/utils/tests/test_estimator_checks.py
+++ b/sklearn/utils/tests/test_estimator_checks.py
@@ -15,9 +15,11 @@
 from sklearn import config_context, get_config
 from sklearn.base import BaseEstimator, ClassifierMixin, OutlierMixin
 from sklearn.cluster import MiniBatchKMeans
-from sklearn.datasets import make_multilabel_classification
+from sklearn.datasets import (
+    load_iris,
+    make_multilabel_classification,
+)
 from sklearn.decomposition import PCA
-from sklearn.ensemble import ExtraTreesClassifier
 from sklearn.exceptions import ConvergenceWarning, SkipTestWarning
 from sklearn.linear_model import (
     LinearRegression,
@@ -46,6 +48,7 @@
     check_array_api_input,
     check_class_weight_balanced_linear_classifier,
     check_classifier_data_not_an_array,
+    check_classifier_not_supporting_multiclass,
     check_classifiers_multilabel_output_format_decision_function,
     check_classifiers_multilabel_output_format_predict,
     check_classifiers_multilabel_output_format_predict_proba,
@@ -79,6 +82,7 @@
 )
 from sklearn.utils.fixes import CSR_CONTAINERS, SPARRAY_PRESENT
 from sklearn.utils.metaestimators import available_if
+from sklearn.utils.multiclass import type_of_target
 from sklearn.utils.validation import (
     check_array,
     check_is_fitted,
@@ -473,6 +477,16 @@ def partial_fit(self, X, y, classes=None, sample_weight=None):
 
 
 class TaggedBinaryClassifier(UntaggedBinaryClassifier):
+    def fit(self, X, y):
+        # X, y = validate_data(self, X, y)
+        y_type = type_of_target(y, input_name="y", raise_unknown=True)
+        if y_type != "binary":
+            raise ValueError(
+                "Only binary classification is supported. The type of the target "
+                f"is {y_type}."
+            )
+        return super().fit(X, y)
+
     # Toy classifier that only supports binary classification.
     def __sklearn_tags__(self):
         tags = super().__sklearn_tags__()
@@ -800,7 +814,6 @@ def test_check_estimator_transformer_no_mixin():
 
 def test_check_estimator_clones():
     # check that check_estimator doesn't modify the estimator it receives
-    from sklearn.datasets import load_iris
 
     iris = load_iris()
 
@@ -809,7 +822,6 @@ def test_check_estimator_clones():
         LinearRegression,
         SGDClassifier,
         PCA,
-        ExtraTreesClassifier,
         MiniBatchKMeans,
     ]:
         # without fitting
@@ -824,7 +836,7 @@ def test_check_estimator_clones():
         with ignore_warnings(category=ConvergenceWarning):
             est = Estimator()
             set_random_state(est)
-            est.fit(iris.data + 10, iris.target)
+            est.fit(iris.data, iris.target)
             old_hash = joblib.hash(est)
             check_estimator(est)
         assert old_hash == joblib.hash(est)
@@ -1420,6 +1432,21 @@ def _more_tags(self):
     check_estimator_tags_renamed("OkayEstimator", OkayEstimator())
 
 
+def test_check_classifier_not_supporting_multiclass():
+    """Check that when the estimator has the wrong tags.classifier_tags.multi_class
+    set, the test fails."""
+
+    class BadEstimator(BaseEstimator):
+        # we don't actually need to define the tag here since we're running the test
+        # manually, and BaseEstimator defaults to multi_output=False.
+        def fit(self, X, y):
+            return self
+
+    msg = "The estimator tag `tags.classifier_tags.multi_class` is False"
+    with raises(AssertionError, match=msg):
+        check_classifier_not_supporting_multiclass("BadEstimator", BadEstimator())
+
+
 # Test that set_output doesn't make the tests to fail.
 def test_estimator_with_set_output():
     # Doing this since pytest is not available for this file.


Code review comments:
diff_hunk: @@ -298,6 +304,17 @@ def type_of_target(y, input_name=""):
     'multilabel-indicator'
     """
     xp, is_array_api_compliant = get_namespace(y)
+
+    def _raise_or_return():
+        """Depending on the value of raise_unknown, either raise an error or return
+        'unknown'.
+        """
+        if raise_unknown:
+            input = input_name if input_name else "data"
+            raise ValueError(f"Unknown label type for {input}: {repr(y)}")
path: sklearn/utils/multiclass.py
text: @user1:
```suggestion
            raise ValueError(f"Unknown label type for {input}: {y!r}")
```

Nitpick, not sure which style people think is better.

@author:
sure, can do, but the first one is much easier to understand to me than the second one, and the second one is also hard to look up for, if you don't know what it does.

diff_hunk: @@ -4737,3 +4752,38 @@ def check_do_not_raise_errors_in_init_or_set_params(name, estimator_orig):
 
         # Also do does not raise
         est.set_params(**new_params)
+
+
+def check_classifier_not_supporting_multiclass(name, estimator_orig):
+    """Check that if the classifier has tags.classifier_tags.multi_class=False,
+    then it should raise a ValueError when calling fit with a multiclass dataset.
+
+    This test is not yielded if the tag is not False.
+    """
+    estimator = clone(estimator_orig)
+    set_random_state(estimator)
+
+    X, y = make_classification(
+        n_samples=100,
+        n_classes=3,
+        n_informative=3,
+        n_clusters_per_class=1,
+        random_state=0,
+    )
+    err_msg = (
+        f"The estimator tag `tags.classifier_tags.multi_class` is False for {name} "
+        "which means it does not support multiclass classification. However, it does "
+        "not raise the right `ValueError` when calling fit with a multiclass dataset, "
+        "including the error message 'Only binary classification is supported.' This "
+        "can be achieved by the following pattern:\n\n"
+        "y_type = type_of_target(y, input_name='y', raise_unknown=True)\n"
+        "if y_type != 'binary':\n"
+        "    raise ValueError(\n"
+        "        'Only binary classification is supported. The type of the target ' \n"
path: sklearn/utils/estimator_checks.py
text: @user1:
```suggestion
        "        'Only binary classification is supported. The type of the target '\n"
```

diff_hunk: @@ -473,6 +477,16 @@ def partial_fit(self, X, y, classes=None, sample_weight=None):
 
 
 class TaggedBinaryClassifier(UntaggedBinaryClassifier):
+    def fit(self, X, y):
+        # X, y = validate_data(self, X, y)
path: sklearn/utils/tests/test_estimator_checks.py
text: @user1:
Is this comment supposed to be here or just forgot to cleanup?