DEP expose y_score instead of y_pred RocCurveDisplay.from_predictions#29865
Conversation
bagustris
changed the title
|
@bagustris Could you solve the issue with the linter. It looks like you did not reformat with the right tool. You should have only a minimum diff (here you modified aorund 50 files). Once this is solve, I'll make a review. Also, you will need for sure to add an entry in the changelog under the |
|
@glemaitre I only edited two files, as shown in first commit d098d12, then run |
|
I still see 64 file changed. Could you revert and avoid any reformatting apart from your changes. |
bagustris
force-pushed
the
fix-roccurverdisplay-args-docs
branch
from
c67cbfa to
cb28366
Compare
|
Done to revert to first commit and then update as needed. |
jeremiedbb
left a comment
jeremiedbb
left a comment
There was a problem hiding this comment.
Thanks for the PR @bagustris, here are a few comments
| """ | ||
| # TODO(1.8): Remove y_pred support and use y_score only | ||
| if y_score is not None and not isinstance(y_pred, str): | ||
| raise ValueError( |
There was a problem hiding this comment.
We need to had new tests to be sure that we properly raise this error.
Basically this is the same thing for the code non-covered in the line below.
There was a problem hiding this comment.
I added unit tests namely test_y_score_and_y_pred_deprecation and test_y_pred_deprecation_warning in test_roc_curve_display.py. I also changed y_pred to y_score in that test file following the proposed changes in this PR.
| y_pred = np.array([0.1, 0.4, 0.35, 0.8]) | ||
|
|
||
| with pytest.warns(FutureWarning, match="y_pred was deprecated in version 1.6"): | ||
| display = RocCurveDisplay.from_predictions(y_true, y_pred=y_pred) |
There was a problem hiding this comment.
The fact that this test is failing with because we are missing y_score means that we have a regression.
@jeremiedbb Do you see a way to get around this issue.
Indeed, I was thinking that you could move the * such that y_score is a keyword only argument as well. However, then we need to define a default for it and thus deprecate it and remove it later.
At least this is the only way that I'm seeing this change possible. And if this is the case, I would be pragmactic and not go with all this hassle for both our users (when checking the documentation) and ourself (to make the maintenance).
Anyt thoughts?
|
@jeremiedbb @glemaitre let me know if further work is needed. |
|
Let me solve the conflict and provide another round of review. |
glemaitre
changed the title
glemaitre
left a comment
glemaitre
left a comment
There was a problem hiding this comment.
I resolved the commit and modify a couple of things:
- add the changelog fragment
- add a default for
y_score(i.e.y_score=None) option for backward compatibility - update the deprecation version
LGTM on my side. @jeremiedbb maybe you want to have another look.
jeremiedbb
left a comment
jeremiedbb
left a comment
There was a problem hiding this comment.
LGTM. The CI failure is unrelated. Thanks @bagustris
…scikit-learn#29865) Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai> Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>
4 participants
Reference Issues/PRs
Fix #29823
What does this implement/fix? Explain your changes.
ytot_trueandy_predtoy_scorein AUC docs.y_predtoy_scoreinRocCurveDisplay.from_predictions(along with a warning for depreciation)Could you check @jeremiedbb @glemaitre?