Skip to content

TST move check_class_weight_balanced_linear_classifier to estimator checks#29712

Merged
glemaitre merged 5 commits intoscikit-learn:mainfrom
adrinjalali:check_class_weight_balanced_linear_classifier
Sep 3, 2024
Merged

TST move check_class_weight_balanced_linear_classifier to estimator checks#29712
glemaitre merged 5 commits intoscikit-learn:mainfrom
adrinjalali:check_class_weight_balanced_linear_classifier

Conversation

@adrinjalali
Copy link
Copy Markdown
Member

This moves check_class_weight_balanced_linear_classifier to estimator checks.

cc @glemaitre @OmarManzoor

@adrinjalali adrinjalali added the Developer API Third party developer API related label Aug 25, 2024
@adrinjalali adrinjalali added this to the 1.6 milestone Aug 25, 2024
@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 25, 2024

✔️ Linting Passed

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

Generated for commit: 4ec83e9. Link to the linter CI: here

Copy link
Copy Markdown
Contributor

@OmarManzoor OmarManzoor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @adrinjalali

From the diff it looks the this check was already present in the estimator checks. Was it not enabled previously?

@OmarManzoor OmarManzoor added the Quick Review For PRs that are quick to review label Aug 27, 2024
Comment on lines +187 to +189
if isinstance(classifier, LinearClassifierMixin):
if "class_weight" in classifier.get_params().keys():
yield check_class_weight_balanced_linear_classifier
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OmarManzoor
yes, the test was present in this file, but not a part of check_estimator function or parametrize_with_checks. So for third party estimators this was not enabled, and our estimators also wouldn't be able to skip it with _xfail_checks. This moves it here like all the other tests.

@glemaitre glemaitre self-requested a review September 2, 2024 09:59
Copy link
Copy Markdown
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a small nitpicks

Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>
@glemaitre glemaitre enabled auto-merge (squash) September 3, 2024 09:35
@glemaitre
Copy link
Copy Markdown
Member

Thanks @adrinjalali. Enabling auto-merge. I might need to come back in case we still have the failure with 32-bits debian build.

@glemaitre glemaitre merged commit d8f0781 into scikit-learn:main Sep 3, 2024
@adrinjalali adrinjalali deleted the check_class_weight_balanced_linear_classifier branch September 3, 2024 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Developer API Third party developer API related module:utils No Changelog Needed Quick Review For PRs that are quick to review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants