FIX Remove redundant yield of check_class_weight_balanced_linear_classifier#33197
Conversation
StefanieSenger
left a comment
There was a problem hiding this comment.
Nice catch, @atheendre130505 / Antigravity AI.
Please try to merge upstream/main into your branch to trigger the CI to run the remaining test.
| @@ -0,0 +1,3 @@ | |||
| - :func:`utils.estimator_checks._yield_classifier_checks` no longer yields | |||
There was a problem hiding this comment.
In the changelog, please refer to a tool from the public API to describe the fix. I think that should be estimator_checks_generator, and maybe more. I leave it to you to research if other API is affected by his fix.
|
Please don't ping people directly by @-mentioning them. Especially if your PR is not yet ready for people to review it. Here are some things to do:
Once all that is done you may request a review from the author of #29712 (not several maintainers). The reason fixing formatting and linter issues is important is that you want to present your work in the best way possible. Messy, unclear, hard to read PRs have a lower probability of being reviewed. This is not because people don't like you or the work but because there are many, many PRs in need of review. After filtering on simple things like "first impression is that this is messy" there are still many PRs left to review. This means investing time in the presentation is a simple thing to do to increase the chances of receiving a review. |
StefanieSenger
left a comment
There was a problem hiding this comment.
Yes, that looks good. Thanks, @atheendre130505.
StefanieSenger
left a comment
There was a problem hiding this comment.
Wait, I need to revert my prior approval. The title of the changelog file is wrong, @atheendre130505.
Please follow the instructions in https://github.com/scikit-learn/scikit-learn/blob/main/doc/whats_new/upcoming_changes/README.md.
| - :func:`utils.estimator_checks.estimator_checks_generator` and | ||
| :func:`utils.estimator_checks.check_estimator` no longer yield | ||
| ``check_class_weight_balanced_linear_classifier`` twice. |
There was a problem hiding this comment.
| - :func:`utils.estimator_checks.estimator_checks_generator` and | |
| :func:`utils.estimator_checks.check_estimator` no longer yield | |
| ``check_class_weight_balanced_linear_classifier`` twice. | |
| - :func:`utils.estimator_checks.estimator_checks_generator` no | |
| longer yields `check_class_weight_balanced_linear_classifier` | |
| twice. |
check_estimator does not yield. It only lists failing checks. I think we don't need to mention it here.
|
I don't think we need a changelog for such a change, so I pushed a commit removing it and enabled auto-merge. |
Reference Issues/PRs
Fixes #33154.
The duplication was accidentally introduced in #29712. This PR implements the suggestion to remove the redundancy and ensure tests continue to pass efficiently.
What does this implement/fix? Explain your changes.
In sklearn/utils/estimator_checks.py, the function _yield_classifier_checks was yielding check_class_weight_balanced_linear_classifier twice for linear classifiers supporting the class_weight parameter.
This resulted in redundant test executions during common estimator checks (e.g., for LogisticRegression), wasting CI/CD time and creating duplicate log entries.
Changes:
Removed the duplicate if block in sklearn/utils/estimator_checks.py.
Verified that the check is now yielded exactly once for linear classifiers and correctly ignored for non-linear classifiers.
Added a changelog entry in doc/whats_new/upcoming_changes/sklearn.utils/.
AI usage disclosure
I used AI assistance for:
Code generation.
Documentation (including examples)
Any other comments?
The fix was verified against several cases:
LogisticRegression (Linear + class_weight): Yields exactly 1 check (Verified).
Linear Classifier (No class_weight): Yields 0 checks (Verified).
Non-Linear Classifier: Yields 0 checks (Verified).
Linear with class_weight=None: Yields 1 check (Verified).
Maintainers: As this addresses duplication introduced in #29712, @ogrisel @adrinjalali @OmarManzoor @glemaitre