Skip to content

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

Merged
glemaitre merged 12 commits into
scikit-learn:mainfrom
mrastgoo:issue-25929-aspect
Aug 23, 2023
Merged

ENH fix x- and y-axis limits and ratio in ROC and PR displays#26366
glemaitre merged 12 commits into
scikit-learn:mainfrom
mrastgoo:issue-25929-aspect

Conversation

@mrastgoo

Copy link
Copy Markdown
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

Charlie-XIAO commented May 14, 2023

Copy link
Copy Markdown
Contributor

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

@glemaitre glemaitre left a comment

Copy link
Copy Markdown
Member

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

github-actions Bot commented Aug 21, 2023

Copy link
Copy Markdown

✔️ Linting Passed

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

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

@glemaitre glemaitre left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

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

I have my eyes on it! 👀

@ArturoAmorQ ArturoAmorQ left a comment

Copy link
Copy Markdown
Member

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 :)

Comment thread doc/whats_new/v1.4.rst Outdated
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
Copy Markdown
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