Skip to content

ENH xlim and ylim to axes of RocCurveDisplay and PrecisionRecallDisplay #26368

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

mrastgoo
Copy link
Contributor

Related to #25929

Adding xlim and ylim of (-0.001, 1.001) to the axes

@Charlie-XIAO
Copy link
Contributor

Charlie-XIAO commented May 14, 2023

With xlim = ylim = (-0.001, 1.001) and setting aspect=1, I'm getting plots like this:

The curve gets cropped, and not clearly distinguishable from the spines. I think we need at least (-0.01, 1.01), see https://gist.github.com/Charlie-XIAO/ecd5173fe0daa9c49f442153ef19e7d1. Let's wait for maintainers to decide.

@ogrisel
Copy link
Member

ogrisel commented May 15, 2023

Thanks for the PR, could you please document your change for version 1.3 under doc/whats_new/v1.3.rst?

@glemaitre
Copy link
Member

Looking at this PR and #26366, it seems that we need to merge PR together. It will simplify the code: By setting first x- and y-lim then, we can directly set the aspect to 1 and avoid using get_data_ratio.

So let's close one of the 2 PRs and change slightly the scope.

With xlim = ylim = (-0.001, 1.001) and setting aspect=1, I'm getting plots like this:

We should indeed take care of the offset to apply. It might be dynamic in case we want the despine option or not. One thing sure, we want it as little as possible but we don't want to crop the line on the display.

@glemaitre glemaitre self-requested a review June 15, 2023 09:15
@glemaitre
Copy link
Member

Closing in favor of #26356

@glemaitre glemaitre closed this Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants