FIX select the probability estimates or transform the decision values when pos_label is provided #18114
Conversation
|
@jnothman @thomasjpfan OK so I think this is the only fixes required. Most probably, nobody encountered this issue :) |
|
Once this PR is merged, I plan to make another one with either an entry in the user guide or an example to show how to pass the |
|
ping @ogrisel as well. |
|
This PR would handle the case where there is a mismatch between I keep on wondering if this behavior of selecting the column would be surprising to a user. |
|
We might also have a similar problem with non-symmetric scorers that take hard class predictions such as f1 score for instance, no? |
Co-authored-by: Olivier Grisel <[email protected]>
Since we don't need to select a column or inverse the decision, we should be safe. But I will add a test. |
|
Some more review comments: |
| # provided. | ||
| raise ValueError(err_msg) | ||
| elif not support_multi_class: | ||
| raise ValueError(err_msg) |
ogrisel
Aug 11, 2020
Member
I do not understand this. In both cases it returns the same message.
Also, what happens when support_multi_class and y_pred.shape[1] != 1?
I do not understand this. In both cases it returns the same message.
Also, what happens when support_multi_class and y_pred.shape[1] != 1?
glemaitre
Aug 18, 2020
Author
Contributor
It was what was intended in the past: https://github.com/scikit-learn/scikit-learn/pull/18114/files#diff-e907207584273858caf088b432e38d04L243-L247
Also, what happens when support_multi_class and y_pred.shape[1] != 1
It is a case where y_true is encoded as binary but that the y_pred` is multi-class. It is a case supported by ROC-AUC apparently and I needed to keep it like this for backward compatibility.
I am not sure that there is a better way of inferring the exact encoding in this case.
It was what was intended in the past: https://github.com/scikit-learn/scikit-learn/pull/18114/files#diff-e907207584273858caf088b432e38d04L243-L247
Also, what happens when support_multi_class and y_pred.shape[1] != 1
It is a case where y_true is encoded as binary but that the y_pred` is multi-class. It is a case supported by ROC-AUC apparently and I needed to keep it like this for backward compatibility.
I am not sure that there is a better way of inferring the exact encoding in this case.
glemaitre
Sep 2, 2020
Author
Contributor
I added an example to be more specific. I think that we should investigate if we can pass labels to type_of_target which could be an optional argument given when we are using it in the metrics.
I added an example to be more specific. I think that we should investigate if we can pass labels to type_of_target which could be an optional argument given when we are using it in the metrics.
|
Another pass |
|
LGTM |
|
I am still very much confused by the multiclass case. Could you please add tests for multiclass classification problem with proba and threshold scorers with string labels? Maybe that would clear the confusion and help me complete this review. |
| # [0. , 0.2 , 0.8 ]]) | ||
| # roc_auc_score( | ||
| # y_true, y_score, labels=[0, 1, 2], multi_class='ovo' | ||
| # ) |
ogrisel
Sep 9, 2020
Member
I still do not understand this condition. This comment refers to the metric function outside of the scorer API.
I assume that this is related to to roc_auc_ovo_scorer which is a _ProbaScorer instance and this condition is about raising a ValueError when y_pred.shape[1] == 1 for some reason but I really do not see how this relates to the example you give here as y_pred has 3 columns in this example so it does not match the case of the condition.
I still do not understand this condition. This comment refers to the metric function outside of the scorer API.
I assume that this is related to to roc_auc_ovo_scorer which is a _ProbaScorer instance and this condition is about raising a ValueError when y_pred.shape[1] == 1 for some reason but I really do not see how this relates to the example you give here as y_pred has 3 columns in this example so it does not match the case of the condition.
thomasjpfan
Sep 9, 2020
Member
I think this is trying to keep the logic from:
scikit-learn/sklearn/metrics/_scorer.py
Line 243
in
0a5af0d
which I added in #15274. This was added because we can have a binary y_true with an estimator trained on > 2 classes.
y_pred.shape[1] == 1 was use to mean y_pred came from a classifier with only one class. The check for the shape of y_pred was added here: 94db3d9
I think this is trying to keep the logic from:
scikit-learn/sklearn/metrics/_scorer.py
Line 243 in 0a5af0d
which I added in #15274. This was added because we can have a binary y_true with an estimator trained on > 2 classes.
y_pred.shape[1] == 1 was use to mean y_pred came from a classifier with only one class. The check for the shape of y_pred was added here: 94db3d9
glemaitre
Oct 5, 2020
Author
Contributor
But in scikit-learn we cannot have a classifier train with a single class, isn't it?
But in scikit-learn we cannot have a classifier train with a single class, isn't it?
thomasjpfan
Oct 5, 2020
Member
It is strange, but it can happen:
from sklearn.tree import DecisionTreeClassifier
from sklearn.datasets import make_blobs
import numpy as np
X, y = make_blobs(random_state=0, centers=2)
clf = DecisionTreeClassifier()
clf.fit(X, np.zeros_like(y))
clf.classes_
# array([0])
It is strange, but it can happen:
from sklearn.tree import DecisionTreeClassifier
from sklearn.datasets import make_blobs
import numpy as np
X, y = make_blobs(random_state=0, centers=2)
clf = DecisionTreeClassifier()
clf.fit(X, np.zeros_like(y))
clf.classes_
# array([0])
glemaitre
Oct 5, 2020
Author
Contributor
Uhm. Then, I am confused with check_classifiers_one_label :) I have to check. Anyway, I think that the last change make everything more explicit.
Uhm. Then, I am confused with check_classifiers_one_label :) I have to check. Anyway, I think that the last change make everything more explicit.
glemaitre
Oct 5, 2020
Author
Contributor
Oh I see a must_pass in the raises. It would be much more explicit to have tag for that I think: accept_single_label for instance.
Oh I see a must_pass in the raises. It would be much more explicit to have tag for that I think: accept_single_label for instance.
|
Otherwise LGTM |
Co-authored-by: Thomas J. Fan <[email protected]>
|
The code is much better organized that previous versions of this PR and the tests are great. Thanks very much for the fix @glemaitre. |
ogrisel
merged commit 193670c
into
scikit-learn:master
… when pos_label is provided (scikit-learn#18114)
… when pos_label is provided (scikit-learn#18114)
Solve a bug where the appropriate column of the probability estimates was not selected or that the decision values where not inversed when
pos_labelis passed and that it does not correspond toclf.classes_[1].