[MRG] FIX Missing test for check_class_weight_balanced_linear_classifier (#…#13313
Conversation
glemaitre
left a comment
There was a problem hiding this comment.
Could you add also an entry in what's new.
In this case, sklearn contribution could be interested to know about an additional common tests.
|
|
||
|
|
||
| def test_check_class_weight_balanced_linear_classifier(): | ||
| class BadBalancedWeightsClassifier(BaseBadClassifier): |
There was a problem hiding this comment.
You can move this class outside next to the other classes.
There was a problem hiding this comment.
Hi @glemaitre, thank you for reviewing this.
I changed the location of the class as you suggested. Can you point me towards where can I find "what's new" to add an entry ?
There was a problem hiding this comment.
|
The error is unrelated (http error). |
…st_check_class_weight_balanced_linear_classifier
doc/whats_new/v0.21.rst
Outdated
| `predict`, `predict_proba`, `transform`, and `decision_function` does not | ||
| change. :issue:`12328` by :user:`Nicolas Hug <NicolasHug>` | ||
|
|
||
| - Add ``test_check_class_weight_balanced_linear_classifier`` to |
There was a problem hiding this comment.
I would simplify a bit the message:
Add an additional common test for estimators to check that ...
There was a problem hiding this comment.
What do you think abou this version:
- Add an additional estimator_check test to verify the proper functioning of the check that corroborates that estimators generate correct balanced class weights (when
class_weight=balanced). :issue:9079by :user:Pavel Soriano <psorianom>.
As I am adding a test for a check, it gets a little bit confusing :/
There was a problem hiding this comment.
oh upsy. Sorry about that. You actually don't need an entry then. Let me change this and merge then
|
Otherwise it looks good |
Co-Authored-By: psorianom <pavelsoriano@fastmail.fr>
|
Actually, we need a second review there. But everything LGTM. |
| # Intentionally modify the balanced class_weight | ||
| # to simulate a bug and raise an exception | ||
| if self.class_weight == "balanced": | ||
| class_weight += 1. |
There was a problem hiding this comment.
I don't get what this is doing. Surely this doesn't result in affecting the output of predict. In fact the reason why the test is passing may be because predict isn't affected by changing class weight at all...
There was a problem hiding this comment.
No, you are right, this dummy class does not affect the output of predict. Instead, it affects class_weight.
I did it that way because, as I understand, the check check_class_weight_balanced_linear_classifier this test is testing does not verify the output of predict. It simply verifies coef_ after the estimator has been fitted.
scikit-learn/sklearn/utils/estimator_checks.py
Line 1994 in bf6949b
|
I see now. Lgtm (can't click approve)
|
|
@psorianom Thanks!!! |
…ear_classifier (scikit-learn#13313)" This reverts commit 12a8b43.
…ear_classifier (scikit-learn#13313)" This reverts commit 12a8b43.
…9079)
Reference Issues/PRs
Fixes #9079
What does this implement/fix? Explain your changes.
check_class_weight_balanced_linear_classifierinsklearn/utils/estimator_checksverifies that a Classifier correctly computes the class weights whenclass_weight="balanced".This check is run on
test_common.py. Nevertheless, it is missing a test insklearn/utils/tests/test_estimator_checks.py.This PR adds a test to verify that the check actually verifies that a classifier is not correctly computing the class weights when
class_weight="balanced".The solution involved generating a dummy class that calculates the balanced weights and then arbitrarily modifies them to simulate the miscalculation of
coef_. The assertion incheck_class_weight_balanced_linear_classifieris then supposed to fail.Any other comments?