TST check that binary only classifiers fail on multiclass data by adrinjalali · Pull Request #29874 · scikit-learn/scikit-learn

adrinjalali · 2024-09-17T15:02:22Z

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

adrinjalali · 2024-09-17T15:03:10Z

Do we need a changelog?

github-actions · 2024-09-17T15:03:47Z

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: 0f4289b. Link to the linter CI: here}

glemaitre · 2024-09-17T16:22:17Z

Do we need a changelog?

I would argue that we should. End-user are not impacted directly but it would be nice to send a signal with an entry to third-party developers.

…s-false

adrinjalali · 2024-09-19T09:48:37Z

sklearn/utils/multiclass.py



-def type_of_target(y, input_name=""):
+def type_of_target(y, input_name="", raise_unknown=False):


I'm tempted to have this always raise. Adding it since we expect estimators to raise an error message, but we don't really have a tool that raises that for the devs.

adrinjalali · 2024-09-19T09:50:36Z

sklearn/utils/tests/test_estimator_checks.py

        LinearRegression,
        SGDClassifier,
        PCA,
-        ExtraTreesClassifier,


removing this cause it's very slow. Removing this reduces the time of this test on my machine from over 7s to 1.7s

An alternative is to set the parameters to have few trees and shallow depth.

Having it doesn't really add anything for the purpose of the test anyway. So I'm happy to have it gone.

adrinjalali · 2024-09-19T09:51:54Z

sklearn/utils/estimator_checks.py

+        " a continuous target is passed and the message should include the word"
+        " 'continuous'"
+    )
+    msg = "Unknown label type: |continuous"


adding the alternative since the code in TaggedBinaryClassifier in this PR should be considered valid code.

adrinjalali · 2024-09-19T09:52:28Z

This is now ready for review.

…s-false

glemaitre

Looks good.

sklearn/utils/multiclass.py

glemaitre · 2024-10-03T10:14:35Z

sklearn/utils/tests/test_estimator_checks.py

        LinearRegression,
        SGDClassifier,
        PCA,
-        ExtraTreesClassifier,


An alternative is to set the parameters to have few trees and shallow depth.

chkoar · 2024-10-11T10:46:45Z

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.

Are you referring to this one?

adrinjalali

Are you referring to this one?

@chkoar generally I'm working on improving all those messages.

adrinjalali · 2024-10-15T11:08:08Z

sklearn/utils/tests/test_estimator_checks.py

        LinearRegression,
        SGDClassifier,
        PCA,
-        ExtraTreesClassifier,


Having it doesn't really add anything for the purpose of the test anyway. So I'm happy to have it gone.

Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>

…s-false

sklearn/utils/multiclass.py

Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>

adrinjalali · 2024-10-16T15:29:45Z

cc @Charlie-XIAO @adam2392 maybe?

adam2392

LGTM except a few nits.

sklearn/utils/multiclass.py

sklearn/utils/tests/test_estimator_checks.py

…s-false

…cikit-learn into tests/tag-multiclass-false

Charlie-XIAO

This LGTM! Just a few more nitpicks:

Charlie-XIAO · 2024-10-16T23:09:05Z

sklearn/utils/estimator_checks.py

+    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'"
+    )


Is this err_msg unused? I think it's more like a comment.

thanks for noticing, yeah now it's shown to the user if the test fails.

sklearn/utils/estimator_checks.py

sklearn/utils/tests/test_estimator_checks.py

Charlie-XIAO · 2024-10-16T23:12:49Z

sklearn/utils/multiclass.py

+        """
+        if raise_unknown:
+            input = input_name if input_name else "data"
+            raise ValueError(f"Unknown label type for {input}: {repr(y)}")


Suggested change

raise ValueError(f"Unknown label type for {input}: {repr(y)}")

raise ValueError(f"Unknown label type for {input}: {y!r}")

Nitpick, not sure which style people think is better.

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.

…s-false

chkoar · 2024-10-18T09:44:43Z

Thanks @adrinjalali

TST check that binary only classifiers fail on multiclass data

35e5cf0

adrinjalali added the Developer API Third party developer API related label Sep 17, 2024

github-actions bot added the module:utils label Sep 17, 2024

adrinjalali added 5 commits September 18, 2024 11:38

changelog

761a366

improve message

56ffc12

Merge remote-tracking branch 'upstream/main' into tests/tag-multiclas…

f16d309

…s-false

blah

df6c943

allow alternative

f3a7744

adrinjalali commented Sep 19, 2024

View reviewed changes

adrinjalali added 4 commits September 19, 2024 12:02

cleanup

26f036e

Merge remote-tracking branch 'upstream/main' into tests/tag-multiclas…

a3d08c6

…s-false

cleanup

2731a6d

Merge remote-tracking branch 'upstream/main' into tests/tag-multiclas…

5ef3d69

…s-false

glemaitre self-requested a review September 28, 2024 11:38

glemaitre reviewed Oct 3, 2024

View reviewed changes

adrinjalali commented Oct 15, 2024

View reviewed changes

adrinjalali and others added 2 commits October 15, 2024 13:09

Update sklearn/utils/multiclass.py

637e695

Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>

Merge remote-tracking branch 'upstream/main' into tests/tag-multiclas…

964095f

…s-false

glemaitre reviewed Oct 16, 2024

View reviewed changes

sklearn/utils/multiclass.py Outdated Show resolved Hide resolved

glemaitre approved these changes Oct 16, 2024

View reviewed changes

adrinjalali and others added 2 commits October 16, 2024 17:29

Update sklearn/utils/multiclass.py

aabfa46

Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>

Merge branch 'main' into tests/tag-multiclass-false

ee5e7fc

adam2392 approved these changes Oct 16, 2024

View reviewed changes

sklearn/utils/multiclass.py Outdated Show resolved Hide resolved

sklearn/utils/tests/test_estimator_checks.py Outdated Show resolved Hide resolved

Adam's review

55692bb

adrinjalali added 2 commits October 16, 2024 20:01

Merge remote-tracking branch 'upstream/main' into tests/tag-multiclas…

bc658ea

…s-false

Merge branch 'tests/tag-multiclass-false' of github.com:adrinjalali/s…

f45d62f

…cikit-learn into tests/tag-multiclass-false

chkoar approved these changes Oct 16, 2024

View reviewed changes

Charlie-XIAO approved these changes Oct 16, 2024

View reviewed changes

adrinjalali added 2 commits October 18, 2024 09:17

Merge remote-tracking branch 'upstream/main' into tests/tag-multiclas…

1a73557

…s-false

Charlie's comments

0f4289b

adrinjalali enabled auto-merge (squash) October 18, 2024 08:20

adrinjalali merged commit 18dc863 into scikit-learn:main Oct 18, 2024

adrinjalali deleted the tests/tag-multiclass-false branch October 18, 2024 09:47



		def type_of_target(y, input_name=""):
		def type_of_target(y, input_name="", raise_unknown=False):

	raise ValueError(f"Unknown label type for {input}: {repr(y)}")
	raise ValueError(f"Unknown label type for {input}: {y!r}")

Uh oh!

Conversation

adrinjalali commented Sep 17, 2024

Uh oh!

adrinjalali commented Sep 17, 2024

Uh oh!

github-actions bot commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✔️ Linting Passed

Uh oh!

glemaitre commented Sep 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrinjalali commented Sep 19, 2024

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chkoar commented Oct 11, 2024

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adrinjalali commented Oct 16, 2024

Uh oh!

adam2392 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Charlie-XIAO left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Charlie-XIAO Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chkoar commented Oct 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

github-actions bot commented Sep 17, 2024 •

edited

Loading

Charlie-XIAO left a comment •

edited

Loading

Charlie-XIAO Oct 16, 2024 •

edited

Loading