ENH despine keyword for ROC and PR curves#26367
ENH despine keyword for ROC and PR curves#26367glemaitre merged 23 commits intoscikit-learn:mainfrom
despine keyword for ROC and PR curves#26367Conversation
|
Do I need to make tests for this? If so, do we add separate tests or parametrize current tests with the additional keyword? |
We would need to make a test to check that the parameter has the desired impact. I think that this is fine to isolate the checks in a new separate test. |
|
#26019 has been merged and I have resolved the merge conflicts. I think there has been no reply for over a week on this PR as well as #26366 and #26368. No hurry but is there any update on this issue @glemaitre? |
|
Since #26366 is now merged, maybe this one is also ready for review? ping @glemaitre |
|
ping @Charlie-XIAO are you still interested in working on this? |
|
Yes sure, I will find some time within these days to resolve the conflicts and make things up-to-date first. |
|
@lucyleeow I've updated :) |
lucyleeow
left a comment
There was a problem hiding this comment.
LGTM! I wonder if it would make sense to use it in one of the examples?
|
Sorry for the delay and thanks for the suggestion @lucyleeow! I've added
CI is strangely failing but I think none of those is related to this PR. |
|
Maybe ping @glemaitre for another review? This is implemented based on your initial design #25929 (comment) |
|
I thought that I reviewed this PR but apparently I did not post my comments. |
glemaitre
left a comment
There was a problem hiding this comment.
LGTM. I just pushed the main code in a small plotting utility function that can evolve over time if we need it somewhere else.
For other display, we might need to be able to specify the bounds but let see when making the subsequent PR.
Reference Issues/PRs
Towards #25929.
What does this implement/fix? Explain your changes.
This PR adds a new keyword
despineto remove the top and right axes in order to make the plot clearer. For examples, please see this gist: https://gist.github.com/Charlie-XIAO/ecd5173fe0daa9c49f442153ef19e7d1Any other comments?
This PR need to have PR #26366 and PR #26368 merged in advance. Also, this is related and may have a lot of conflicts with PR #26019.