TST add unit tests for current _get_response #21041
Conversation
sklearn/metrics/_plot/base.py
Outdated
| @@ -111,7 +113,7 @@ def _get_response(X, estimator, response_method, pos_label=None): | |||
| pos_label = estimator.classes_[1] | |||
| y_pred = y_pred[:, 1] | |||
| else: | |||
| class_idx = np.flatnonzero(estimator.classes_ == pos_label) | |||
| class_idx = np.flatnonzero(estimator.classes_ == pos_label)[0] | |||
There was a problem hiding this comment.
This feels like a bug fix. Should we include this in a whats_new?
There was a problem hiding this comment.
This comment is on an outdated line but the point is still valid.
There was a problem hiding this comment.
This is a private function so I think that we are fine.
There was a problem hiding this comment.
Even tho we have different output shapes for y_pred, all the functions that call _get_response also ends up calling _binary_clf_curve. _binary_clf_curve will then force y_score to be 1d:
scikit-learn/sklearn/metrics/_ranking.py
Line 734 in 07a9dc9
So in the end, not a bug fix (for a public API).
| except ValueError: | ||
| pass |
There was a problem hiding this comment.
May we add a comment on what version numpy will raise a ValueError?
There was a problem hiding this comment.
I will add a comment. NumPy will not raise an error. Scikit-learn raise an error because pos_label="unknown". And the NumPy warning is only raised if pos_label is a string. I did not think about it but we could maybe generate the warning when pos_label is a valid string label.
There was a problem hiding this comment.
Actually, I remove the test since we completely change the way to get the index.
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
|
Failing test is on AppVeyor which should not be running. |
glemaitre
moved this from WAITING FOR REVIEW
to REVIEWED AND WAITING FOR CHANGES
in Guillaume's pet
glemaitre
moved this from REVIEWED AND WAITING FOR CHANGES
to MERGED
in Guillaume's pet
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Working on #21038, I could catch an inconsistency regarding the shape of
y_predwhenpos_labelis set to a value.We were also raising a
FutureWarningwith a scalar/NumPy arrayinoperation.This PR adds minimum tests. They will be probably adapted later on in #20999 but this is safer to have them now.