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. |
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.