Skip to content

Enhance ROC Curve Display Tests for Improved Clarity and Maintainability #31266

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 10 commits into from
Apr 30, 2025

Conversation

NEREUScode
Copy link
Contributor

PR Description:

Summary of Changes:

This PR refactors the data_binary fixture in the test_roc_curve_display.py file. The previous fixture filtered a multiclass dataset (Iris) to create a binary classification task. However, this approach resulted in AUC values consistently reaching 1.0, which does not reflect real-world challenges.

The new fixture utilizes make_classification from sklearn.datasets to generate a synthetic binary classification dataset with the following characteristics:

  • 200 samples and 20 features.
  • 5 informative features and 2 redundant features.
  • 10% label noise (flip_y=0.1) to simulate real-world imperfections in the data.
  • Class separation (class_sep=0.8) set to avoid perfect separation.

These changes provide a more complex and representative dataset for testing the roc_curve_display function and other related metrics, thereby improving the robustness of tests.

Reference Issues/PRs:


For Reviewers:

  • This change ensures that the dataset used for testing is more reflective of real-world data, particularly in classification tasks that may involve noise and less clear separation between classes.

Replaced the `data_binary` fixture that filtered classes from a multiclass dataset 
with a new fixture generating a synthetic binary classification dataset using 
`make_classification`. This ensures consistent data characteristics, introduces 
label noise, and better simulates real-world classification challenges.
Copy link

github-actions bot commented Apr 28, 2025

✔️ Linting Passed

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

Generated for commit: 90077ae. Link to the linter CI: here

@NEREUScode
Copy link
Contributor Author

@lucyleeow i guess everything is good now

Copy link
Member

@lucyleeow lucyleeow left a comment

Choose a reason for hiding this comment

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

LGTM! My only nit is that I am not sure we need 20 features but I'll let the 2nd reviewer decide that.

@lucyleeow lucyleeow added the Waiting for Second Reviewer First reviewer is done, need a second one! label Apr 29, 2025
@NEREUScode
Copy link
Contributor Author

@lucyleeow, I added 20 features to prevent overfitting. Without them we'd likely get a perfect ROC AUC of 1.0. Despite the added features, training time remains fast, so performance isn't a concern

Copy link

@marcin-okon marcin-okon left a comment

Choose a reason for hiding this comment

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

LGTM

@lucyleeow
Copy link
Member

@NEREUScode thanks for explaining. What AUC do you get with 20 features? And what AUC do you get with e.g., 10?

@adrinjalali
Copy link
Member

@lucyleeow 's question stands, but looks good anyway.

@adrinjalali adrinjalali merged commit d51f17b into scikit-learn:main Apr 30, 2025
36 checks passed
@NEREUScode
Copy link
Contributor Author

@lucyleeow I'll run more tests to see if the feature number needs adjusting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use more complex data in test_roc_curve_display.py
5 participants