Skip to content

ENH fix x- and y-axis limits and ratio in ROC and PR displays #26366

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

Merged
merged 12 commits into from
Aug 23, 2023

Conversation

mrastgoo
Copy link
Contributor

related to #25929

We set the ratio to 1/ ax.get_data_ratio() to get square plot
It is related to point 3 as discussed in the issue #25929

@Charlie-XIAO
Copy link
Contributor

Charlie-XIAO commented May 14, 2023

Is this what we want? I think we want to set aspect to 1, especially for PR curves (see https://gist.github.com/Charlie-XIAO/ecd5173fe0daa9c49f442153ef19e7d1). With your ratio I'm getting PR curve plots like this:

@glemaitre glemaitre changed the title ENH fix aspect ratio of RocCurveDisplay and PrecisionRecalDisplay to equal ENH fix x- and y-axis limits and ratio in ROC and PR displays Jun 15, 2023
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I run the example that would be modified and it looks good locally.
We can check if this is also the case with the CIs.

@glemaitre glemaitre self-requested a review August 21, 2023 12:20
@github-actions
Copy link

github-actions bot commented Aug 21, 2023

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: ab9f4ca. Link to the linter CI: here

Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the rendering on the generated documentation and this is fine.

Ready for merging.

@ArturoAmorQ do you want to make a pass such that we merge this one?

@glemaitre glemaitre self-requested a review August 21, 2023 13:53
@ArturoAmorQ
Copy link
Member

@ArturoAmorQ do you want to make a pass such that we merge this one?

I have my eyes on it! 👀

Copy link
Member

@ArturoAmorQ ArturoAmorQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nitpick, after which it LGTM :)

Co-authored-by: Arturo Amor <86408019+ArturoAmorQ@users.noreply.github.com>
@glemaitre glemaitre merged commit bd9b0f6 into scikit-learn:main Aug 23, 2023
@glemaitre
Copy link
Member

Merging thanks all.

akaashpatelmns pushed a commit to akaashp2000/scikit-learn that referenced this pull request Aug 25, 2023
…-learn#26366)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Arturo Amor <86408019+ArturoAmorQ@users.noreply.github.com>
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Aug 29, 2023
…-learn#26366)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Arturo Amor <86408019+ArturoAmorQ@users.noreply.github.com>
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
…-learn#26366)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Arturo Amor <86408019+ArturoAmorQ@users.noreply.github.com>
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