-
-
Notifications
You must be signed in to change notification settings - Fork 25.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FIX Fix handling of binary_only tag in check_estimator #17812
Conversation
15edf2e
to
522ed74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, thank you @brcharron !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat! This may deserve a what's new entry? Not sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change is big enough to be added to whats new:
Please add an entry to the change log at doc/whats_new/v0.24.rst
with tag |Fix|. Like the other entries there, please reference this pull request with :pr:
and credit yourself (and other contributors if applicable) with :user:
.
8fece24
to
524717d
Compare
Thanks for the reviews! I added an entry to the What's New page. |
Thank you @brcharron ! |
Thanks @brcharron 🎉 |
…17812) Co-authored-by: Bruno Charron <bruno@charron.email>
…17812) Co-authored-by: Bruno Charron <bruno@charron.email>
Reference Issues/PRs
Fixes #16798
What does this implement/fix? Explain your changes.
Some checks in
check_estimator
were generating test cases with more than 2 classes regardless of whether thebinary_only
tag was provided by the estimator. Fixed these checks so they only generate test cases with up to 2 classes. Also simplified other tests which had special behavior for thebinary_only
tag by leveraging_enforce_estimator_tags_y
. Changed the test to useSGDClassifer
as it has a apartial_fit
method which did not pass the existing test.Any other comments?